#375 Response stream corruption when a request is cancelled

[Duracell1989]

I've ran into the same issue as #375; this code works in my project.
No more IOExceptions after cancelling the token.

[msftclas]

All CLA requirements met.

[Duracell1989]

I want to use this package, but I need this fix for it to work. Can this PR be merged?

[galvesribeiro]

LGTM

[galvesribeiro]

@jterry75 any thoughts?

[dgvives]

@Duracell1989 did you experience this problem of stream not having <CR/LF> and execution being trapped there?
#434

[Duracell1989]

@Duracell1989 did you experience this problem of stream not having <CR/LF> and execution being trapped there?
#434

No, the problem was that when cancelling; it would throw IOExceptions.

[Duracell1989]

@jterry75; Rebased with the latest master- thus, resolved all the merge conflicts.

[jterry75]

@Duracell1989 - Super sorry I thought that #481 fixed this but now I looked back and realized we only fixed one of the three similar methods. Given the change we did there by removing the stream.Dispose on task cancellation we no longer should ever see the ObjectDisposedException. I think you want to follow the pattern for the WithCancellation we did in the other one on the ReadLineAsync() call to unblock the task right?

[dgvives]

I wouldn't recommend WithCancellation extension, produces different results depending on docker response time, causing tests to fail on CI/CD pipeline tests. I know I pushed it, didn't know better then Get Outlook for Android<https://aka.ms/ghei36>

…

________________________________ From: Justin <notifications@github.com> Sent: Tuesday, February 2, 2021 5:57:36 PM To: dotnet/Docker.DotNet <Docker.DotNet@noreply.github.com> Cc: David García Vives <david.garcia.vives@hotmail.com>; Comment <comment@noreply.github.com> Subject: Re: [dotnet/Docker.DotNet] #375 Response stream corruption when a request is cancelled (#394) @Duracell1989<https://github.com/Duracell1989> - Super sorry I thought that #481<#481> fixed this but now I looked back and realized we only fixed one of the three similar methods. Given the change we did there by removing the stream.Dispose on task cancellation we no longer should ever see the ObjectDisposedException. I think you want to follow the pattern for the WithCancellation we did in the other one on the ReadLineAsync() call to unblock the task right? — You are receiving this because you commented. Reply to this email directly, view it on GitHub<#394 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ARATUN5AIKYGYUSNCTRM3HTS5AVIBANCNFSM4HNNFE6Q>.

[Duracell1989]

As suggested by @dgvives: I removed the WithCancellation and have rewritten this to the same cancellation as the rest of the code.

[dgvives]

@Duracell1989 apologies, I didn't explain myself properly

WithCancellation extension method can not be just removed as it is required to cancel MonitorStreamForMessagessAsync

Commit d7372c0 didn't pass the tests, being cancelled after 360m build timeout on the CI/CD pipeline.
Details

That I have in mind is to replace WithCancellation extension by using TaskCompletionSource to exit loop but remove exception being thrown.
Original idea was @suneption on #343, I just incorporated the changes required to pass CI/CD pipeline test.

After some unexpected results, I just updated this part avoid exceptions from uncertain execution frames.

        internal static async Task MonitorStreamForMessagesAsync<T>(Task<Stream> streamTask, CancellationToken cancel, IProgress<T> progress)
        {

            var tcs = new TaskCompletionSource<bool>();

            using (cancel.Register(() => tcs.TrySetCanceled(cancel)))
            using (var stream = await streamTask)
            using (var reader = new StreamReader(stream, new UTF8Encoding(false)))
            using (var jsonReader = new JsonTextReader(reader) { SupportMultipleContent = true })
            {
                while (await await Task.WhenAny(jsonReader.ReadAsync(default), tcs.Task))
                {
                    var ev = Serializer.Deserialize<T>(jsonReader);
                    progress.Report(ev);
                }
            }
        }

Haven't pushed this yet as it is pending tests.
BTW, I checked your idea on checking Token.IsCancellationRequested and it was working on the two previously updated methods, 👍 .

[Duracell1989]

@dgvives; you're going to add my cancel.IsCancellationRequested change to another branch of you?
Or; do you want me to make some changes to approve this issue and accept the PR?

[dgvives]

@Duracell1989 I won't add your changes to the branch I'm currently working on, I had to learn it is considered bad contributor etiquette.
I suggest you rollback to 9f18f36, their tests results were fine.

Besides this it would be nice to include some test case that calls the modified method.
I'm working on tests cases for that piece of code I posted before, and won't submit PR for it until tests are added.

You decide to go ahead with tests and like to see any example, I'm happy to help, just ping me.

[dgvives]

@Duracell1989 by looking at your modification for MonitorStreamForMessagesAsync method, looks like condition cancel.IsCancelationRequested won't be hit as it exits earlier when OperationCanceledException is thrown in .WithCancelation extension method

Have you tested MonitorStreamAsync and MonitorResponseForMessagesAsync methods after your changes? How did you do it?
It's being a challenge to test those

[Duracell1989]

@dgvives I didn't test that specific change - also because I found it a challenge. Let me revert that one in that function.

[dgvives]

@Duracell1989 I closed #487, tests results were not the same on all executions.

[Duracell1989]

@dgvives; Yeah, I know how to run tests. But if I run all the tests; there is this 1 tests that fail. But if you run that failing test on it's own. If passed. So, some data isn't getting cleared between multiple tests.
I hoped it was only on my machine - but it is also on the build agent. So, I'm still investigating why this is happening.

[Duracell1989]

I think we have a race-condition failure:
By running multiple tests, the _onMessageCalled function isn't called (no idea why) and the Assert.True(wasProgressCalled); fails.
When running the MonitorEventsAsync_Succeeds test separate, the _onMessageCalled function is getting called, so here the Assert.True(wasProgressCalled); succeeded.

[dgvives]

I've had the same problem while trying to validate issues were solved.
Initially my intention was to fix WaitContainerAsync call but it turned to be more complicated than I expected.

I ran into the same problems you faced, different test results on different executions for the same tests, tests running fine locally but failing on CI/CD pipeline, tests running ok individually but failing when all together, tests never ending and causing TimeoutException on build agent.

[Duracell1989]

I've fixed this by adding the Thread.Sleep to the test. This way, we give the Stream some time do report back some progress; see here

[jterry75]

@Duracell1989 / @dgvives - Which of these changes are we taking looks like both are still being actively worked on?

[dgvives]

These changes were merged into #487 and it continued from there until tests success

[jterry75]

Sounds good can we close this one?

[Duracell1989]

Best practice is to merge a PR if it adds value. Waiting for another PR which could improve more, isn't great, because you never know when this other PR will be approved (or never at all).

In this case; the #487 has been closed and no timeline is present when other improvements will be finished.
So, I would suggest to merge this - to at least resolve the IO Exception and later; in another PR we could improve upon this, if that is needed.

[jterry75]

@Duracell1989 / @dgvives - The other was closed are we wanting to take this one first and work out the rest later?

[dgvives]

@jterry75 I hit a wall being unable to cancel MonitorStreamForMessagesAsync call with cancellation token, and I'm not sure how long it's going to take me to find a proper solution, as there are custom handlers and stream readers involved.

There is a reason behind calling Stream.Dispose(), once JsonTextReader starts reading, it won't stop until end of stream or full json token is read.
This is a problem when trying to cancel dockerClient.Container.GetContainerLogs using ContainerLogsParameters{ Follor = true }, or dockerClient.Container.GetContainerStats using ContainerStatsParameters{ Stream = true }

The stream is continuous and there is no way to stop reading because the reader is using calls to stream.Read() method and this won't stop once finished.

I'm reviewing implementation on source code for these methods, but it's taking a while. I can't recommend removing stream.Dispose until a tested alternative have been found

[Duracell1989]

From what I understand:
You register a anonymous function which does a stream.Dispose() on the CancellationToken. This means that the Stream get's dispose as soon as the cancellation is requested.
But, the problem here is that the Stream could still be in use (still reading) within the while loop and still get's accessed but it's already disposed of (because of the anonymous function registered on the CancellationToken.
Because the Stream is within a using statement (for example: using (var stream = await streamTask) and using (var stream = await response.Content.ReadAsStreamAsync())) it doesn't need to be disposed of immediately when the CancellationToken is cancelled. That's why checking the !cancel.IsCancellationRequested makes sure we exit the while loop (and not using the Stream anymore) and after that (because of the using statement) the Stream get's disposed of at the correct time, when we're stopped using the Stream.

[jterry75]

@Duracell1989 - The stream.Dispose() had the intent of unblocking the outstanding read to ReadAsStreamAsync since no cancellation is supported don this read. Its not really about making sure to dispose the object its about releasing the wait. I dont see how the !cancel.IsCancellationRequested is unblocked until the next read comes in which may be some unknown amount of time in the future right?

[dgvives]

Found the problem

Basically, deep inside it goes inside a while(read()) with no possibility to exit by using CancellatonToken
I has ended up being much more complicated than it seemed

Detailed explanation here #487 (comment)