CancellationToken not cancelling #487

dgvives · 2021-02-03T00:03:31Z

Fixes #236
Fixes #375
Fixes #416
Fixes #434
Closes #343
Closes #351
Closes #394

It solves execution stop on DockerClient.Containers.WaitContainerAsync waiting for the task to finish without being able to cancel.

var waitContainerTask = dockerClient.Containers.WaitContainerAsync(createContainerResponse.ID, cts.Token);

Task can be cancelled using token or default timeout.
Included tests to verify cancellation works both ways.
Included tests to several endpoints calling this method.
Fixed tests failing on different execution contexts due to new exceptions being thrown.

dgvives · 2021-02-03T00:04:19Z

Working solution pending tests cases.

dgvives · 2021-02-06T20:46:30Z

Closed, tests produce different results on each execution

dgvives · 2021-02-07T03:05:38Z

Updated tests to include timeout

dgvives · 2021-02-14T17:27:44Z

Included tests to validate issues solution.

dgvives · 2021-02-15T23:24:26Z

Core changes are set to a minimum. However, some issues are complicated to test.
New test cases have been added to validate issue solution, and a common test fixture put in place to avoid collisions between test monitoring events.
All core modifications have been tested and tests are included within this PR.

@jterry75 @galvesribeiro would you have a look on it?

jterry75 · 2021-02-16T19:04:08Z

src/Docker.DotNet/Endpoints/StreamUtil.cs

                    {
-                        string line;
-                        while ((line = await reader.ReadLineAsync()) != null)
+                        string line = await await Task.WhenAny(reader.ReadLineAsync(), tcs.Task);


Fascinating idea. You seem to have an extra await though.

Yes, it looks like that.

await Task.WhenAny(task1, task2) would return either task1 or task2.
Then, this task1 or task2 need to be awaited too.

Original idea from #343 by @suneption, I just adjusted it for tests to pass.

Crazy. I did not know you could inline await :).

jterry75 · 2021-02-16T19:05:06Z

src/Docker.DotNet/Endpoints/StreamUtil.cs


        internal static async Task MonitorStreamAsync(Task<Stream> streamTask, DockerClient client, CancellationToken cancel, IProgress<string> progress)
        {
+            var tcs = new TaskCompletionSource<string>();
+            cancel.Register(() => tcs.TrySetCanceled(default));


Why not in a using like the others?

Should be like others by next commit. I'm running tests now before pushing again

jterry75

Just a couple of questions. Looks good

jterry75 · 2021-02-17T22:52:32Z

src/Docker.DotNet/Endpoints/StreamUtil.cs

+            {
+                while (!cancel.IsCancellationRequested)
+                {
+                    string line = await await Task.WhenAny(reader.ReadLineAsync(), tcs.Task);


Alright one last question. When tcs.Task is selected the 2nd await will throw because there is no result. I assume this is the correct behavior and we want to throw the cancellation exception?

~~When cancelling a token I expect to get an OpertionCancelledException. Otherwise it would mean task reached its end instead of being cancelled.~~

~~I have no knowledge about architecture or system designs, i just enjoy thinks working on the way I like. Do the minimum to not do it twice.~~

~~I've included several tests on this, expecting OperationCancelledException to be thrown and failing test otherwise.~~
~~There should be on IContainerOperationsTests~~

Edited: @jterry75 before removing Stream.Dispose, an ObjectDisposedException would be thrown most of the time, but also IOException and other unexpected behaviours. It took me a while to figure out how to test some parts, in my opinion no changes should be added without tests supporting them. Also there is so much more fun on it

~~Edit again:~~
~~By using (cancel.Register(() => tcs.TrySetCanceled(default))) causes tcs.Task not to be selected because of finished but because of cancelled. Then, OperationCancelledException is thrown.~~

~~A different use of TaskCompletionSource is on GetContainerLogs_MonitorStream_Succeeds test, where I validate task result to be what's being set within MonitorStream handler~~

@galvesribeiro wins

I don't think it will throw there. The tcs.Task will be returned.

In this case I wouldn't assign to the line variable directly. I would assign it to a Task and would check which task is that - the readline or the cancellation.

If that is the cancellation task, then yeah, it is cancelled. If it is the deadline Task, then you string line = await readLineTask();.

There should be a test behind this
Edit: @galvesribeiro you win

I listened to your advice. It's not exactly as you said but shares the same idea.
It helped fixing some problems when reading streams from containers created without a tty
Thank you @galvesribeiro

jterry75

LGTM

jterry75 · 2021-02-17T22:53:32Z

@galvesribeiro - Will you check my last question about throwing the cancellation exception here. As long as your good with that we can take this change IMO.

dgvives · 2021-02-18T00:50:30Z

@jterry75 @galvesribeiro to not merge this. Tested behavior is not right on MonitorStreamAsync

MonitorStreamForMessagesAsync, MonitorResponseForMessagesAsync, and MonitorStreamAsync behave in the same way, throwing TaskCancelledException when task is cancelled.

Expect unified behavior to be pushed soon

dgvives · 2021-02-20T20:34:40Z

Some tests have output details, I used them to understand what I was doing. At some point I gave up but test output details remain

Emdot · 2021-02-21T23:21:43Z

test/Docker.DotNet.Tests/IContainerOperationsTests.cs

+        //  @dgvives: This test doesn't work on xUnit test project. Execution never finishes.
+        //            Same code on consoleApp works just fine


This may suggest a deadlock bug in the code, rather than an xUnit problem. It's common for async code to work in Console apps but break elsewhere, due to the difference in synchronization contexts. Unfortunately, I'm not an expert in how sync contexts and schedulers and such work, so I don't know where to suggest looking.

I see, I'm creating some sort of web app to test this. Will update with test results

@Emdot you are righ, JsonReader.ReadAsync can't be cancelled once serialization has started.

Looking for an alternative now

Emdot · 2021-02-21T23:59:33Z

src/Docker.DotNet/DockerClient.cs

            if (timeout != s_InfiniteTimeout)
            {
                using (var timeoutTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
                {
                    timeoutTokenSource.CancelAfter(timeout);


Would it make sense to move the timeout complexity to a separate function? Then this whole method can be:

var request = PrepareRequest(method, path, queryString, headers, data); return RunWithTimeoutAsync( ct => _client.SendAsync(request, completionOption, ct), cancellationToken, timeout);

A RunWithTimeoutAsync implementation might look like this:

public static async Task<T> TimeoutAfter<T>(Func<CancellationToken, Task<T>> makeTask, CancellationToken inputToken, TimeSpan timeout) { // Make a token that triggers either manually or when the input token triggers. using var cts = CancellationTokenSource.CreateLinkedTokenSource(inputToken); var timerTask = Task.Delay(timeout, cts.Token); var mainTask = makeTask(cts.Token); // Start both the timeout and the main task in parallel. Task firstTask == await Task.WhenAny(timerTask, mainTask).ConfigureAwait(false); // At this point, one of them has finished. Cancel the other one. cts.Cancel(); // If the consumer canceled, put the output task in the canceled state and link it to the input token. if (inputToken.IsCancellationRequested) throw new OperationCanceledException(inputToken); // If the timeout occurred first, throw a timed-out exception. if (firstTask == timerTask) throw new TimeoutException(); // Otherwise return the result of the main task. If it was canceled, this may throw // a cancelation exception. If it faulted, it may throw some other exception. return await mainTask.ConfigureAwait(false); }

For added performance you could use a state parameter to avoid the closure. As a bonus, timeouts emit a separate exception than cancellation.

@Emdot great suggestion.
I'll wait for this to get approved and continue with improvements.

dgvives · 2021-02-23T01:45:47Z

~~@jterry75 moving back to stream.Dispose() solved cancellation problems while monitoring streams.~~
~~I added a custom handler to have same kind of exception thrown on token cancellation.~~
~~Tests have been added to validate failing tasks under different circumstances.~~

~~Reason behind this changeset was to fix just DockerClient.Containers.WaitContainerAsync, it should be ready for review.~~

dgvives · 2021-02-24T23:09:31Z

Closed while research on stream.read not being cancelled is done

dgvives · 2021-02-26T18:31:56Z

When using JsonSerializer.Deserialize<T>(JsonReader) to deserialize, deep inside it is calling ExpandoObject.ReadReadObject(JsonReader) and no cancellation is possible from inside the loop, there is no token to cancel.

I went over the source for JsonReader, StreamReader, Stream, and finally JsonSerializer until I found out this.

https://github.com/JamesNK/Newtonsoft.Json/blob/b6dc05be5a0f4808f06ec430f3bb59b24d3fbc3e/Src/Newtonsoft.Json/Converters/ExpandoObjectConverter.cs#L113#L141

        private object ReadObject(JsonReader reader)
        {
            IDictionary<string, object?> expandoObject = new ExpandoObject();

            while (reader.Read())
            {
                switch (reader.TokenType)
                {
                    case JsonToken.PropertyName:
                        string propertyName = reader.Value!.ToString();

                        if (!reader.Read())
                        {
                            throw JsonSerializationException.Create(reader, "Unexpected end when reading ExpandoObject.");
                        }

                        object? v = ReadValue(reader);

                        expandoObject[propertyName] = v;
                        break;
                    case JsonToken.Comment:
                        break;
                    case JsonToken.EndObject:
                        return expandoObject;
                }
            }

            throw JsonSerializationException.Create(reader, "Unexpected end when reading ExpandoObject.");
        }

This isn't easy to find out, was difficult to figure out a solution, and turned even more complicated to test.

jterry75 · 2021-03-02T23:31:53Z

@dgvives - Looks like you got it!

dgvives · 2021-03-03T10:19:39Z

@jterry75 - Yes, it has been a very interesting journey into task cancelling and tests

jterry75 · 2021-03-03T17:54:03Z

Do you want to rebase all your commits into one just because it was a lot of change over them and then I'll do a final review and signoff. But looks good to me

dgvives · 2021-03-05T14:15:02Z

@jterry75 done

jterry75

LGTM

jterry75 · 2021-03-05T21:29:28Z

@galvesribeiro - Want to take a look as well

galvesribeiro

Awesome work! Thank you for the contribution and the huge patience across the reviews.

galvesribeiro · 2021-03-05T21:56:26Z

@jterry75 do you think we have enough for a new release now?

galvesribeiro · 2021-04-05T17:08:16Z

@jterry75 ?

dgvives closed this Feb 6, 2021

dgvives mentioned this pull request Feb 6, 2021

#375 Response stream corruption when a request is cancelled #394

Closed

dgvives reopened this Feb 7, 2021

dgvives marked this pull request as ready for review February 14, 2021 17:22

jterry75 reviewed Feb 16, 2021

View reviewed changes

dgvives requested a review from jterry75 February 16, 2021 20:56

jterry75 reviewed Feb 17, 2021

View reviewed changes

jterry75 approved these changes Feb 17, 2021

View reviewed changes

dgvives requested review from galvesribeiro and jterry75 February 20, 2021 19:59

dgvives mentioned this pull request Feb 20, 2021

ISystemOperations.AuthenticateAsync now returns AuthResponse. #496

Open

Emdot reviewed Feb 21, 2021

View reviewed changes

dgvives closed this Feb 24, 2021

dgvives mentioned this pull request Feb 25, 2021

Start of the new user layer: client, daemon, registries #497

Closed

dgvives reopened this Feb 26, 2021

Squash commit

f0336bf

dgvives force-pushed the bugfix/cancellation-token-not-cancelling branch from 974e624 to f0336bf Compare March 3, 2021 18:32

jterry75 approved these changes Mar 5, 2021

View reviewed changes

galvesribeiro approved these changes Mar 5, 2021

View reviewed changes

galvesribeiro merged commit c0bca61 into dotnet:master Mar 5, 2021

drleq mentioned this pull request Aug 30, 2021

New release after one year? #526

Closed

dgvives deleted the bugfix/cancellation-token-not-cancelling branch May 16, 2022 14:19

		// @dgvives: This test doesn't work on xUnit test project. Execution never finishes.
		// Same code on consoleApp works just fine

CancellationToken not cancelling #487

CancellationToken not cancelling #487

Conversation

dgvives commented Feb 3, 2021 • edited Loading

dgvives commented Feb 3, 2021

dgvives commented Feb 6, 2021

dgvives commented Feb 7, 2021

dgvives commented Feb 14, 2021

dgvives commented Feb 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jterry75 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgvives Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgvives Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jterry75 left a comment

Choose a reason for hiding this comment

jterry75 commented Feb 17, 2021

dgvives commented Feb 18, 2021

dgvives commented Feb 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgvives Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Emdot Feb 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgvives commented Feb 23, 2021 • edited Loading

dgvives commented Feb 24, 2021

dgvives commented Feb 26, 2021 • edited Loading

jterry75 commented Mar 2, 2021

dgvives commented Mar 3, 2021

jterry75 commented Mar 3, 2021

dgvives commented Mar 5, 2021

jterry75 left a comment

Choose a reason for hiding this comment

jterry75 commented Mar 5, 2021

galvesribeiro left a comment

Choose a reason for hiding this comment

galvesribeiro commented Mar 5, 2021

galvesribeiro commented Apr 5, 2021

dgvives commented Feb 3, 2021 •

edited

Loading

dgvives Feb 17, 2021 •

edited

Loading

dgvives Feb 17, 2021 •

edited

Loading

dgvives Feb 22, 2021 •

edited

Loading

Emdot Feb 21, 2021 •

edited

Loading

dgvives commented Feb 23, 2021 •

edited

Loading

dgvives commented Feb 26, 2021 •

edited

Loading