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

Possible error in usage of cancellationToken parameter to context.CreateTimer in AzureStorageScenarioTests.cs? #1178

Open
erikbra opened this issue Jan 6, 2025 · 2 comments

Comments

@erikbra
Copy link

erikbra commented Jan 6, 2025

Referring to

https://github.com/Azure/durabletask/blob/main/test/DurableTask.AzureStorage.Tests/AzureStorageScenarioTests.cs#L3151, which is:

 Task timeoutTask = context.CreateTimer(deadline, cts.Token);

The second parameter of context.CreateTimer, that should be the state, not the cancellationToken, am I right? I'm trying to understand how to use this in my own projects, and looking at the source code to understand.

In this particular scenario, the state returned from the CreateTimer is not used, so it might not be easy to spot, but shouldn't the line be:

 object dummyState = new object() // whatever, the result is not used in this example
 Task timeoutTask = context.CreateTimer(deadline, dummyState, cts.Token);

where dummyState is the state? If not, the timer will never be cancelled, even when the approval task completes, and the orchestration instance will never finish?

Or have I misunderstood how this works?

@cgillum
Copy link
Member

cgillum commented Jan 6, 2025

I think you're right. In fact, passing a cancellation token as the state parameter can be quite dangerous since deserializing cancellation tokens can result in memory corruption. We'll look into fixing this test code and maybe adding a runtime check for this mistake. Thanks for bringing this up.

@erikbra
Copy link
Author

erikbra commented Jan 6, 2025

Nice, thanks a lot for your input! It aligns with what I see in my own code too. The timer never gets cancelled, even if I cancel the CancellationTokenSource, if I use it as in the example. However, if I pass a(ny) state as the 2nd parameter, and the cancellation token as the last parameter, things work as expected

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

No branches or pull requests

2 participants