Skip to content
This repository was archived by the owner on Aug 9, 2021. It is now read-only.

WIP Add locks to protect concurrent rules edition #604

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

btry
Copy link
Contributor

@btry btry commented Jul 12, 2018

Experimental - Add locks to protect concurrent rules edition

The purpose is to avoid simultaneous modification of rules in some specific cases.

Changes description

Checklist

Please check if your PR fulfills the following specifications:

  • Tests for the changes have been added
  • Docs have been added/updated

Estimated time

Assignee 🍅
@btry 2

References

Closes #N/A
Related #N/A
Depends on #N/A

@btry btry added the bug label Jul 12, 2018
@btry btry added this to the 2.1 milestone Jul 12, 2018
@btry btry self-assigned this Jul 12, 2018
@btry btry requested review from DIOHz0r and ingluife July 12, 2018 07:31
accesslint[bot]
accesslint bot previously approved these changes Jul 12, 2018
@btry btry force-pushed the feature/protect_rules_creation branch 2 times, most recently from bb2f238 to 4f01cdf Compare July 12, 2018 08:32
@ingluife
Copy link
Contributor

Hi, @btry.

The source to EntityRuleEditionException is correct?

use GlpiPlugin\Flyvemdm\Exception\EntityRuleEditionException;

@btry
Copy link
Contributor Author

btry commented Jul 12, 2018

Hi @ingluife

yes this is correct. If you read the PR to see the purpose is to ensure atomicity in some critical blocks of code (GLPI does not provides transactions yet, this is on its way)

@DIOHz0r
Copy link
Contributor

DIOHz0r commented Jul 13, 2018

What do you mean with "modification of rules"? or in Which scenario this happens?

@btry
Copy link
Contributor Author

btry commented Jul 13, 2018

Consider there are 2 parallel executions to add an invitation. Due t o the lack of atomicity (transaction) for now, one process may create a rule to create computers in an entity while the other may do the same (with tthe same destination entity).

In such scenario here is what might happen

  • Process A searches for a rule to import a computer in entity 1 and gets 0 result
  • process B searches for a rule to import an a computer in the entity 1 and gets 0 result
  • process A creates the rule because it does not exists yet
  • Process B creates the rule because it does not exists yet

We finally have 2 rules doing the same (destination entity = 1) but we require to have only one.

Same type of scenario when removing criterias. If no criteria exists the rule is deleted. But this is risky if an other process tries to re-use the rule because it needs it.

We need locks to prevent this. The failure in unit tests you encouteered may be related to the lack of locks. Also I found il my local database duplicates rules making some tests fail. That's why I asked you if you also had duplicates rows (yesterday ?)

@btry btry force-pushed the feature/protect_rules_creation branch from 4f01cdf to d4af333 Compare July 13, 2018 15:42
ingluife
ingluife previously approved these changes Jul 13, 2018
@ajsb85 ajsb85 changed the title WIP - experimental - Add locks to protect concurrent rules edition WIP Add locks to protect concurrent rules edition Jul 13, 2018
@btry
Copy link
Contributor Author

btry commented Jul 17, 2018

FYI: glpi-project/glpi@ba83cca

@btry btry force-pushed the feature/protect_rules_creation branch from 14c4da6 to 0157ca9 Compare August 23, 2018 15:24
@btry btry force-pushed the feature/protect_rules_creation branch 2 times, most recently from d7381af to 5d4fdd3 Compare August 31, 2018 07:04
@ajsb85 ajsb85 requested review from ajsb85 and Naylin15 September 21, 2018 09:15
@btry btry force-pushed the feature/protect_rules_creation branch from 5d4fdd3 to d968de6 Compare September 23, 2018 09:25
The purpose is to avoid simultaneous modification of rules in some specific cases

Signed-off-by: Thierry Bugier <[email protected]>
@btry btry force-pushed the feature/protect_rules_creation branch from d968de6 to 3a14404 Compare September 24, 2018 21:37
@ajsb85 ajsb85 closed this Dec 18, 2018
@ajsb85 ajsb85 reopened this Dec 18, 2018
@ajsb85 ajsb85 changed the base branch from develop to features/for_later December 18, 2018 00:23
@btry btry closed this Dec 20, 2018
@btry btry changed the base branch from features/for_later to develop December 20, 2018 17:42
@btry btry reopened this Dec 20, 2018
@ajsb85 ajsb85 assigned DIOHz0r and unassigned btry Dec 21, 2018
@ajsb85 ajsb85 modified the milestones: 2.1, 3.0 Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants