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.

Sign up for GitHub

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

[Foundation] Fix memory leak when using sync methods. #8555

Closed
wants to merge 4 commits into from
Closed

[Foundation] Fix memory leak when using sync methods. #8555

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da7517e
[Foundation] Fix memory leak when using sync methods.
mandel-macaque May 9, 2020
0d15014
Remove not needed check.
mandel-macaque May 11, 2020
bb2f268
Merge branch 'master' of github.com:xamarin/xamarin-macios into memor…
mandel-macaque May 11, 2020
6cbf9b7
Readding the lock. Do not loose the keys.
mandel-macaque May 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 2 additions & 14 deletions src/Foundation/NSUrlSessionHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ protected override async Task<HttpResponseMessage> SendAsync (HttpRequestMessage
var nsrequest = await CreateRequest (request).ConfigureAwait(false);
var dataTask = session.CreateDataTask (nsrequest);

var tcs = new TaskCompletionSource<HttpResponseMessage> ();
var tcs = new TaskCompletionSource<HttpResponseMessage> (TaskCreationOptions.RunContinuationsAsynchronously);

lock (inflightRequestsLock) {
#if !MONOMAC && !__WATCHOS__
Expand Down Expand Up @@ -673,21 +673,10 @@ public override void DidCompleteWithError (NSUrlSession session, NSUrlSessionTas
void SetResponse (InflightData inflight)
{
lock (inflight.Lock) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might not need the lock anymore. It's not clear the (only) other usage is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do agree, and I was tempted to remove it, but did not want to add a more complicated change over the one that fixed the leak.

if (inflight.ResponseSent)
return;

if (inflight.CancellationTokenSource.Token.IsCancellationRequested)
return;

if (inflight.CompletionSource.Task.IsCompleted)
return;

var httpResponse = inflight.Response;

inflight.ResponseSent = true;

// EVIL HACK: having TrySetResult inline was blocking the request from completing
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this hack was necessary in the first place, exactly what was blocking? If some other code is waiting for the CompletionSource to complete, and executes code when that happens, it's that code's responsibility to not block.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was not the person that wrote this. But in the tests, the lock + the TrySetResult blocks meaning that the request would never complete.

Copy link
Contributor

Choose a reason for hiding this comment

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

There has been a few other threading fixes in the past. It's possible that the original condition does not happen anymore, but it's also possible it's just harder to reproduce. How do you intend to test this ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is already tested, all the tests in https://github.com/xamarin/xamarin-macios/blob/master/tests/monotouch-test/System.Net.Http/MessageHandlers.cs will timeout and fail if the call blocks. The blocking happens both on async and sync calls. There is no need to add a specific test for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Our test suite has historically not been good to catch threading errors, like missing locks. I'm not so much afraid of the EVIL HACK removal than the rest.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are keeping the lock, so no change there. The lock was added if there is more than one thread accessing the inflight data, I don't think it will happen. The task.run was added due to #8555 (comment) but moving the token creation to be async fixes the lock and the memory leak (in managed objects).

Copy link
Contributor

Choose a reason for hiding this comment

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

The change is less risky than it was. Still, with recent changes, I don't think we have the correct setup to test this properly (which goes beyond unit testing). c.c. @chamons

Task.Run (() => inflight.CompletionSource.TrySetResult (httpResponse));
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this is leaking. The closure will be collected by the GC when the Task.Run method completes.

Copy link
Member Author

Choose a reason for hiding this comment

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

The closure should be GC, but it is not. Look at #8350 (comment) profiling results. The leak is a memory
stream from the httpcontent of the response. Removing the use of a lambda, fixes the issue.

The leak only happens when it is used in a sync method, so in async the lambda is correctly GC, in sync is not. I could not pin point the exact reason on why the GC is not doing the job correctly.

The code was wrong nevertheless.

inflight.CompletionSource.TrySetResult (inflight.Response);
}
}

Expand Down Expand Up @@ -829,7 +818,6 @@ class InflightData : IDisposable
public HttpResponseMessage Response { get; set; }

public Exception Exception { get; set; }
public bool ResponseSent { get; set; }
public bool Errored { get; set; }
public bool Disposed { get; set; }
public bool Completed { get; set; }
Expand Down