Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition on stop #1081

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mamidenn
Copy link
Contributor

Describe the change
No longer pass the global CancellationToken to persistence operations that could take place after initiating a host shutdown. This prevents cancelling workflows or stopping the host from cancelling writing persistence operations and thus losing data.

Fixes #953
Fixes #1032

Describe your implementation or design
Removed the CancellationToken parameter from persistence operations in finally blocks where TaskCancelledExceptions could be handled.

Tests
Yes. I reproduced the issue by adding a StopScenario for persistence providers. After the change, this scenario passes.

Breaking change
No.

@mamidenn mamidenn force-pushed the fix-race-condition-on-stop-min branch 2 times, most recently from 27cb70f to 0444177 Compare August 16, 2022 07:17
@mamidenn mamidenn marked this pull request as draft August 16, 2022 07:22
@mamidenn mamidenn force-pushed the fix-race-condition-on-stop-min branch 4 times, most recently from 7fc1474 to dd0ddf9 Compare August 16, 2022 07:34
@mamidenn mamidenn marked this pull request as ready for review August 16, 2022 09:10
@@ -10,7 +10,7 @@ public class SqliteCollection : ICollectionFixture<SqliteSetup>

public class SqliteSetup : IDisposable
{
public string ConnectionString { get; set; }
public static string ConnectionString { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to be made static?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to have simple access to the value from the new SqliteStopScenario as well as aligning it with the way all the other scenario setups have static connection strings.

@mamidenn mamidenn force-pushed the fix-race-condition-on-stop-min branch 2 times, most recently from 645d7f6 to fa4f738 Compare August 12, 2023 15:16
@mamidenn mamidenn marked this pull request as draft August 12, 2023 15:21
If the host is stopped as soon as WorkflowCompleted LifeCycleEvent is
raised, some persistence providers cannot persist the 'Completed' state
in time.
Do not pass global CancellationToken to persistence operations on host
shutdown. This way it is ensured that persistence operations are not
cancelled.
@mamidenn mamidenn force-pushed the fix-race-condition-on-stop-min branch from fa4f738 to f150cd4 Compare August 12, 2023 15:32
@mamidenn mamidenn marked this pull request as ready for review August 12, 2023 15:37
@mamidenn
Copy link
Contributor Author

Sorry for taking so long. I removed everything that was not strictly necessary to make the new tests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants