Skip to content

Conversation

tillig
Copy link
Member

@tillig tillig commented Jul 12, 2023

This allows non-async/await and Task-based handling to complete and still remove the scope without the try/finally removing it before the task is finished.

Based on comments for #17.

This allows non-async/await and Task-based handling to complete and still remove the scope without the try/finally removing it before the task is finished.
@tillig
Copy link
Member Author

tillig commented Jul 12, 2023

@russellfoster @srogovtsev Give this a look, I think I got it. If you don't have time, no biggie, but thought you'd be interested. Plus, I have to admit my Task semantics are a little rusty since async/await came along. I had to come up with some pretty contrived examples to run tests on it.

@tillig tillig requested a review from alistairjevans July 12, 2023 17:11
alistairjevans
alistairjevans previously approved these changes Jul 12, 2023
Copy link
Member

@alistairjevans alistairjevans left a comment

Choose a reason for hiding this comment

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

Approved with two questions. 👍

@srogovtsev
Copy link

You'd also need a test for cancelled tasks, something like:

var cancellationTokenSource = new CancellationTokenSource();
cancellationTokenSource.Cancel();

var fakeHandler = new FakeInnerHandler(_ =>
        {
            return Task.Factory.StartNew<HttpResponseMessage>(async () =>
            {
                //this should cancel immediately
                await Task.Delay(-1, cancellationTokenSource.Token);
                //we never get here, but the compiler doesn't know it
               return new HttpResponseMessage(HttpStatusCode.OK);
            });
        });

@tillig
Copy link
Member Author

tillig commented Jul 12, 2023

  • Added continuation options to specify the scheduler and that the continuation should run synchronously.
  • Added async/await test equivalents of the Task tests. This made the original test for scope removal redundant because the same scenario is being tested with the new tests.

@tillig
Copy link
Member Author

tillig commented Jul 13, 2023

Pushing this through. I think I got everything and it should at least unblock folks who were affected by the issue. We can tweak which scheduler to use later if we need to.

@tillig tillig merged commit f749654 into develop Jul 13, 2023
@tillig tillig deleted the feature/fix-task-handling branch July 13, 2023 15:43
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

Successfully merging this pull request may close these issues.

3 participants