Skip to content

[AndroidClientHandler] Prevent orphaned thread in DoProcessRequest#344

Merged
grendello merged 2 commits intodotnet:masterfrom
leonluc-dev:master
Jan 5, 2017
Merged

[AndroidClientHandler] Prevent orphaned thread in DoProcessRequest#344
grendello merged 2 commits intodotnet:masterfrom
leonluc-dev:master

Conversation

@leonluc-dev
Copy link
Copy Markdown
Contributor

Followup to #321

In the current implementation the WaitOne task will never finish if the cancellationToken is never cancelled.
This results in an orphaned task/thread.

This can be solved by creating a CancellationTokenSource for the WaitOne thread and linking it to the original cancellation token. This CancellationTokenSource can then be canceled after the task completes or raises and exception, making sure the WaitOne task runs to completion and is properly disposed of every run of this method.

@monojenkins
Copy link
Copy Markdown

Hello! I'm the build bot for the Mono project.

I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done.

Contributors can ignore this message.

@jonpryor
Copy link
Copy Markdown
Contributor

@grendello: Can you review?

@gameleon-dev: Would it be possible to create a unit test for this? It doesn't seem like it would require a dedicated server endpoint, so it seems like it should be possible to unit test this...

Unit tests would go into src/Mono.Android/Test/Xamarin.Android.Net/AndroidClientHandlerTests.cs.

@leonluc-dev
Copy link
Copy Markdown
Contributor Author

leonluc-dev commented Dec 29, 2016

While it seems difficult to unit test the issue this pull request addresses (orphaned thread), it should be possible to test if the cancellation requests are handled properly with something like the following psuedo code:

    [Test]
    public void Cancel_Client_Works()
    {
      var cts = new CancellationTokenSource();
      cts.Cancel(); //Cancel immediately
      using (var c = new HttpClient(CreateHandler())) {
        Task tr = c.GetAsync("http://10.255.255.1", cts.Token);
        Assert.Throws<System.OperationCanceledException>(tr.Wait(), "SHOULD NOT HAPPEN: Handler did not throw cancellation exception");
       }
    }

    [Test]
    public void Timeout_Client_Works()
    {
      var cts = new CancellationTokenSource(5000); //Cancel after 5000ms
      using (var c = new HttpClient(CreateHandler())) {
        Task tr = c.GetAsync("http://10.255.255.1", cts.Token);
        Assert.Throws<System.OperationCanceledException>(tr.Wait(), "SHOULD NOT HAPPEN: Handler did not throw cancellation exception");
        Assert.IsTrue(cts.IsCancellationRequested, "SHOULD NOT HAPPEN: The request cancelled before cancellation was requested");
      }
    }

I will try to write a unit test based on the above psuedo code and add it to this pull request.

@grendello grendello merged commit ff86ed8 into dotnet:master Jan 5, 2017
@github-actions github-actions Bot locked and limited conversation to collaborators Feb 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants