Prevent concurrent object access#426
Draft
yhabteab wants to merge 4 commits into
Draft
Conversation
We don't need to reload them that early as the request can still fail before even starting to process it in `incident#ProcessEvent`, so move the incident recipients restoration code into `ProcessEvent` to avoid superfluous DB query in error cases.
The built-in `delete` function can handle nil values gracefully, so the extra nil check and map lookup aren't necessary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR restructures the
ObjectandIncidentlookups and creations a bit to prevent some of the issues we were having as described in the referenced issues. Previously, the event processing logic goes like this:Objectinto the database in its own transaction. While this is happening, the entire objects cache is locked, so any other request that targeting a completely different object will be blocked until the transaction is committed.Incidentand all the related records in a single and long transaction. And of course, while this is happening, the incident object is locked, so any other request that targeting the same incident will be blocked until the ongoing one completes.This PR changes the logic to the following:
incident.ProcessEventfunction, and acquire the lock for that incident, we will then start a DB tx to sync the incident and all the related records just like before, but this time, the object syncing is included in this very same tx. When we receive another request that targeting the same object, it will be blocked inincident.ProcessEventuntil the ongoing one completes, but other requests can freely proceed without such blocking.There is one difference between the old and new logic that is worth mentioning. Previously, if we fail to successfully sync the object into the database, it wasn't added to the cache at all, as the cache is only updated after the tx is committed. That's not the case anymore, but since incidents are created/cached in the same way (even with the main branch), I don't have any better way to handle this without introducing more complexity. If in doubt, we can always add some cleanup logic or the like to remove dead objects and incidents from the cache in a later/separate PR if we find it necessary.
resolves #250
resolves #266
Blocked By
events,incident_eventsto DB #423