Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Make LocalOnlyLock reentrant #3513

Merged
merged 1 commit into from
Mar 11, 2023
Merged

Conversation

marosmars
Copy link
Contributor

Pull Request type

  • [ X] Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes (Please run ./gradlew generateLock saveLock to refresh dependencies)
  • WHOSUSING.md
  • Other (please describe):

NOTE: Please remember to run ./gradlew spotlessApply to fix any format violations.

Changes in this PR

It did not support reentrancy.
This was a problem at least in following case:

worker -> updateTask -> throw new TerminateWorkflowException -> terminateWorkflow

The lock was acquired twice during above execution order. With no reentrancy, the second lock (wf terminateion) failed and sweeper had to wait until the first lock (task update) was released and only then workflow could be terminated.

Alternatives considered

No alternatives, this was a clear break of the API contract as defined in the Lock service

It did not support reentrancy.
This was a problem at least in following case:

worker -> updateTask -> throw new TerminateWorkflowException ->
terminateWorkflow

The lock was acquired twice during above execution order. With no
reentrancy, the second lock (wf terminateion) failed and sweeper had to wait until the
first lock (task update) was released and only then workflow could be
terminated.

Signed-off-by: Maros Marsalek <mmarsalek@frinx.io>
@marosmars marosmars force-pushed the reentrant_local_lock branch from 5dd1ab3 to eb55096 Compare March 7, 2023 08:54
@marosmars
Copy link
Contributor Author

Hey @v1r3n , can we get a quick look on this small fix ?

Thx, Maros

@v1r3n v1r3n merged commit ad1c785 into Netflix:main Mar 11, 2023
@marosmars marosmars deleted the reentrant_local_lock branch March 13, 2023 14:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants