[Foundation] Ensure that we do not block when in the background without a background session. #5463

mandel-macaque · 2019-01-23T12:12:20Z

The mono threadpool gets into an unknown state when the application goes
into the background. This fix allows the task that are inflight to be
canceled when the app goes to the background allowing the application
not to hang and letting the developer retyr the request.

If a developer needs to work with the app in the background, he should
be using a background session, this fix just ensures that we are left in
a known state but does not mean that developers should use this kind of
sessions.

The MessageHandler class is just used in Mac OS X and does not have the
idea of the app going to the background, therefore the fix is not needed
in that handler.

mandel-macaque · 2019-01-23T12:12:37Z

For background info about the fix, please read #5334

monojenkins · 2019-01-23T12:33:52Z

Build failure
❌ Build was aborted

🔥 Build failed 🔥

mandel-macaque · 2019-01-23T12:38:02Z

weird, this compiled locally with no issues..

monojenkins · 2019-01-23T13:05:14Z

Build failure
❌ Build was aborted

rolfbjarne · 2019-01-23T13:48:00Z

src/Foundation/NSUrlSessionHandler.cs

+			// therefore, we do not have to listen to the notifications.
+			if (string.IsNullOrEmpty (configuration.Identifier)) {
+				using (var notification = new NSString ("UIApplicationWillResignActiveNotification"))
+					notificationToken = NSNotificationCenter.DefaultCenter.AddObserver (notification, BackgoundNotificationCb);


The observer is never removed, which means the BackgroundNotificationOn delegate contained in the observer will keep a permanent reference to the NSUrlSessionHandler instance, and it will never be collected by the garbage collector.

that string constant is already a field/notification inside UIApplication, WillResignActiveNotification
any reason why you can't use it ?

Yes, we are not compiling the class at a point in time in which we have access to it. Please take a look at the first commit build error in the bots :/

That error looks like it was because it's compiling for watchOS where UIApplication is not available.

https://developer.apple.com/documentation/uikit/uiapplicationwillresignactivenotification?language=objc does not mention watchOS has this constant.

Using the name manually you're working around that - but it's unlikely the code works one watchOS (the code just make it looks like it should)

I made that change locally and got the following compilation error:

System.Net.Http/HttpClientHandler.platformnotsupported.cs(129,63): warning CS1998: This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread. HttpClientEx.cs(83,8): warning CS0219: The variable 'webExceptionStatus' is assigned but its value is never used /Users/mandel/Xamarin/xamarin-macios/master/xamarin-macios/src/Foundation/NSUrlSessionHandler.cs(181,87): error CS0117: 'UIApplication' does not contain a definition for 'WillResignActiveNotification' make[3]: *** [../../class/lib/monotouch_watch/reference/System.Net.Http.dll] Error 1

Nevertheless, I'll add the !WATCH to show that it is not done on WatchOS.

the error just means it's not available on watchOS - your code avoid that (at compile time) but it won't change runtime (and you're also exposing a constant string that is not part of the public API on the platform which is, at best, risky).

src/Foundation/NSUrlSessionHandler.cs

spouliot · 2019-01-23T16:14:03Z

src/Foundation/NSUrlSessionHandler.cs

+			// therefore, we do not have to listen to the notifications.
+			if (string.IsNullOrEmpty (configuration.Identifier)) {
+				using (var notification = new NSString ("UIApplicationWillResignActiveNotification"))
+					notificationToken = NSNotificationCenter.DefaultCenter.AddObserver (notification, BackgoundNotificationCb);


that string constant is already a field/notification inside UIApplication, WillResignActiveNotification
any reason why you can't use it ?

src/Foundation/NSUrlSessionHandler.cs

monojenkins · 2019-01-24T13:29:32Z

Build success
✅ Build succeeded
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
✅ Generator Diff (no change)
✅ Test run succeeded

src/Foundation/NSUrlSessionHandler.cs

monojenkins · 2019-01-25T11:40:13Z

Build failure
❌ Build was aborted

monojenkins · 2019-01-25T12:49:06Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
✅ Generator Diff (no change)
🔥 Test run failed 🔥

Test results

1 tests failed, 0 tests skipped, 80 tests passed.

Failed tests

monotouch-test/iOS Unified 32-bits - simulator/Debug (static registrar): Failed

spouliot · 2019-01-28T14:04:40Z

src/Foundation/NSUrlSessionHandler.cs

+		void AddNotification ()
+		{
+			if (!isBackgroundSession && notificationToken == null)
+				using (var notification = new NSString ("UIApplicationWillResignActiveNotification"))


same comments as earlier:

why not use UIApplication.WillResignActiveNotification instead of your own constant ? Minimally it avoid the memory allocation and any chance of a typo. Also if it ever gets obsoleted we'll not get a compiler warning to update this code.

why not follow the advice on the field ? "Use UIApplication.Notifications.ObserveWillResignActive helper method instead." ?

a backgorund session. The mono threadpool gets into an unknown state when the application goes into the background. This fix allows the task that are inflight to be canceled when the app goes to the background allowing the application not to hang and letting the developer retyr the request. If a developer needs to work with the app in the background, he should be using a background session, this fix just ensures that we are left in a known state but does not mean that developers should use this kind of sessions. The MessageHandler class is just used in Mac OS X and does not have the idea of the app going to the background, therefore the fix is not needd in that handler.

mandel-macaque · 2019-02-06T13:13:58Z

@GouriKumari The following steps can be used to reproduce the issue and the fix:

Get the sample application from iOS HttpClient GetAsync hangs indefinitely #5333
Launch the application and wait a few seconds to see that requests are performed.
Set the application to the background and wait LONGER than 10 seconds.
Get back to the application and ensure that requests continue to happen.

We should check setting the app to the background by:

Switching an other app to the foreground.
Go to the home screen.

The testes should see that the requests, when the app goes to the background fail, and that when we get back to the app, it is not stuck and requests continue happening.

monojenkins · 2019-02-06T15:11:55Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
✅ Generator Diff (no change)
🔥 Test run failed 🔥

Test results

1 tests failed, 0 tests skipped, 80 tests passed.

Failed tests

introspection/Mac Unified: Failed (Test run crashed (exit code: 134).)

mandel-macaque · 2019-02-06T16:18:04Z

As soon as we have a package I'll post the link here for QA.

monojenkins · 2019-02-06T23:18:48Z

Build success
✅ Build succeeded
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
✅ Generator Diff (no change)
✅ Test run succeeded

mandel-macaque · 2019-02-07T10:17:51Z

You can ignore the VSTS issues :)

p-lad · 2019-02-07T10:30:52Z

Hello @mandel-macaque @GouriKumari ,

Following is the status of the pr : -
As mentioned on slack I have reproduced issue on Stable XI build which are as follows:-
Xamarin.Mac Version: 5.2.1.13 (Visual Studio Enterprise)
Xamarin.iOS Version: 12.2.1.13 (Visual Studio Enterprise)

Application output logs:-
https://gist.github.com/p-lad/50ee0c2329d11ce1747e9d0600fe603f

Screen cast link:-
http://recordit.co/N1Yz5Vq2Fc

===============================================================
Following is the status on Fixed build mentioned in pr:-
Build taken : -
XI :-
https://bosstoragemirror.blob.core.windows.net/wrench/jenkins/handler-cancel-background/ec0c6e740d0eaffd838c3f52dd943a2811dceb4e/1/package/xamarin.mac-5.7.0.128.pkg

XM:-
https://bosstoragemirror.blob.core.windows.net/wrench/jenkins/handler-cancel-background/ec0c6e740d0eaffd838c3f52dd943a2811dceb4e/1/package/xamarin.ios-12.7.0.128.pkg

Build info:
Xamarin.Mac Version: 5.7.0.128 (Visual Studio Enterprise)
Xamarin.iOS Version: 12.7.0.128 (Visual Studio Enterprise)

Application output logs:-
https://gist.github.com/p-lad/7aabb998e19f6d91e4b812a1485011c7

Screen cast link:-
http://recordit.co/Z4twf2VqDf

As discussed with @mandel-macaque on slack, app went to the background and got back, all threads started normally with the logs stating that new ones were created.

Hence from QA side it is QA-Approved.

Let us know if anything is required from our side.

Thanks.

…andler for thethreadpool. When an application was moved the the background, the thread pool from mono would be left in an unknonw state, making applications to stale (https://xamarin.github.io/bugzilla-archives/58/58633/bug.html#c7). This work was added in dotnet#5463 We now set the default behaviour to skip the workaround to see if the new provided mono works as expected. We do not fully remove the workaround because we need some real world testing. If the new ThreadPool from mono does not work as expected we do provide a property to re-add the workaround. The BypassBackgroundSessionCheck can be set to false to allow users get it back. The following is an example usage of the API: ```csharp // create your own handler instance rather than using the one provided by default. var handler = new NSUrlSessionHandler() { BypassBackgroundSessionCheck = false, // readd the hack }; var httpClient = new HttpClient (handler); // use the handler with the hack ``` This is a partial fix for dotnet#7080

…andler for thethreadpool. (#8296) When an application was moved the the background, the thread pool from mono would be left in an unknonw state, making applications to stale (https://xamarin.github.io/bugzilla-archives/58/58633/bug.html#c7). This work was added in #5463 We now set the default behaviour to skip the workaround to see if the new provided mono works as expected. We do not fully remove the workaround because we need some real world testing. If the new ThreadPool from mono does not work as expected we do provide a property to re-add the workaround. The BypassBackgroundSessionCheck can be set to false to allow users get it back. The following is an example usage of the API: ```csharp // create your own handler instance rather than using the one provided by default. var handler = new NSUrlSessionHandler() { BypassBackgroundSessionCheck = false, // readd the hack }; var httpClient = new HttpClient (handler); // use the handler with the hack ``` This is a partial fix for #7080

The iOS NSUrlSessionHandler needed to use a workaround to ensure that Http requests that were performed by the application did not end up in a stale situation in which the completion would never be reached. This was due to dotnet/macios#5463 This workound will listen to the UIApplication notifications and would cancel all the inflight requests. For reference, this happens here: https://github.com/xamarin/xamarin-macios/blob/main/src/Foundation/NSUrlSessionHandler.cs#L174 As you can see, if we set the property to false, we will get the notification. This is **not longer needed** as mono fixed the runtime issue. The Xamairn.iOS team skips that workaround by default as per this commit: dotnet/macios@b6fbae5#diff-1e7e2b8b4f4fe98a485a36c085a0e9cb94f53fa87a3ff11c7c54682cc3b9d514 This library should not be using the workaround because it will have undesired side effects on applications because they will have cancelation exceptions that are not expected AND the library does not provide an API to reuse the HttpClient instance configured by the client.

mandel-macaque requested review from chamons, rolfbjarne and spouliot as code owners January 23, 2019 12:12

mandel-macaque changed the title ~~[Foundation] Ensure that we do not block when in the backgorund without~~ [Foundation] Ensure that we do not block when in the background without Jan 23, 2019

mandel-macaque changed the title ~~[Foundation] Ensure that we do not block when in the background without~~ [Foundation] Ensure that we do not block when in the background without a backgorund session. Jan 23, 2019

rolfbjarne requested changes Jan 23, 2019

View reviewed changes

mandel-macaque added the requires-qa-before-merge The pull request requires QA to approve it before it can be merged label Jan 23, 2019

spouliot suggested changes Jan 23, 2019

View reviewed changes

mandel-macaque mentioned this pull request Jan 24, 2019

CFnetworkHandler does not support background sessions #5469

Closed

rolfbjarne reviewed Jan 24, 2019

View reviewed changes

src/Foundation/NSUrlSessionHandler.cs Outdated Show resolved Hide resolved

rolfbjarne reviewed Jan 25, 2019

View reviewed changes

src/Foundation/NSUrlSessionHandler.cs Show resolved Hide resolved

rolfbjarne approved these changes Jan 25, 2019

View reviewed changes

spouliot reviewed Jan 28, 2019

View reviewed changes

mandel-macaque added 6 commits February 6, 2019 12:38

Do not use property since it is not present at that compilation point.

f34899f

Clean code a little.

05f95c8

Address reviews.

97131a5

Remove the notification if the inflight data is 0.

944bf27

Ensure the token is set to null when removed.

9bd39d3

rolfbjarne changed the title ~~[Foundation] Ensure that we do not block when in the background without a backgorund session.~~ [Foundation] Ensure that we do not block when in the background without a background session. Feb 6, 2019

Move to use the property rather than a NSString.

50c92a2

mandel-macaque force-pushed the handler-cancel-background branch from 7d272ea to 50c92a2 Compare February 6, 2019 13:08

rolfbjarne approved these changes Feb 6, 2019

View reviewed changes

Merge branch 'master' into handler-cancel-background

ec0c6e7

spouliot approved these changes Feb 6, 2019

View reviewed changes

mandel-macaque removed the requires-qa-before-merge The pull request requires QA to approve it before it can be merged label Feb 7, 2019

mandel-macaque merged commit e84e027 into dotnet:master Feb 7, 2019

mandel-macaque deleted the handler-cancel-background branch February 7, 2019 11:53

mandel-macaque mentioned this pull request Feb 7, 2019

iOS HttpClient GetAsync hangs indefinitely #5333

Closed

alanag13 mentioned this pull request Jul 9, 2019

NSUrlSession implementation of HttpClient throws TaskCanceledException immediately after backgrounding #6443

Closed

wadebaird mentioned this pull request Sep 8, 2019

TaskCanceledException raised when attempting an HttpClient.GetAsync #6936

Closed

migueldeicaza mentioned this pull request Sep 23, 2019

Remove workaround for ThreadPool not working #7080

Closed

jahmai-ca mentioned this pull request Sep 27, 2019

Xamarin.iOS Threadpool randomly halts when app is suspended and doesn't recover when resumed mono/mono#12318

Closed

mandel-macaque mentioned this pull request Apr 6, 2020

[Foundation] As a default, do not use the workaround in NSUrlSessionHandler for the threadpool. #8296

Merged

mandel-macaque mentioned this pull request Apr 16, 2021

[iOS] Do not use the background check in the iOS handler. AzureAD/microsoft-authentication-library-for-dotnet#2555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Foundation] Ensure that we do not block when in the background without a background session. #5463

[Foundation] Ensure that we do not block when in the background without a background session. #5463

mandel-macaque commented Jan 23, 2019 •

edited

Loading

mandel-macaque commented Jan 23, 2019

monojenkins commented Jan 23, 2019

mandel-macaque commented Jan 23, 2019

monojenkins commented Jan 23, 2019

rolfbjarne Jan 23, 2019

spouliot Jan 23, 2019

mandel-macaque Jan 24, 2019

spouliot Jan 24, 2019

mandel-macaque Jan 25, 2019

spouliot Jan 25, 2019

spouliot Jan 23, 2019

monojenkins commented Jan 24, 2019

monojenkins commented Jan 25, 2019

monojenkins commented Jan 25, 2019

Failed tests

spouliot Jan 28, 2019

mandel-macaque commented Feb 6, 2019

monojenkins commented Feb 6, 2019

Failed tests

mandel-macaque commented Feb 6, 2019

monojenkins commented Feb 6, 2019

mandel-macaque commented Feb 7, 2019

p-lad commented Feb 7, 2019

[Foundation] Ensure that we do not block when in the background without a background session. #5463

[Foundation] Ensure that we do not block when in the background without a background session. #5463

Conversation

mandel-macaque commented Jan 23, 2019 • edited Loading

mandel-macaque commented Jan 23, 2019

monojenkins commented Jan 23, 2019

mandel-macaque commented Jan 23, 2019

monojenkins commented Jan 23, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jan 24, 2019

monojenkins commented Jan 25, 2019

monojenkins commented Jan 25, 2019

Test results

Failed tests

Choose a reason for hiding this comment

mandel-macaque commented Feb 6, 2019

monojenkins commented Feb 6, 2019

Test results

Failed tests

mandel-macaque commented Feb 6, 2019

monojenkins commented Feb 6, 2019

mandel-macaque commented Feb 7, 2019

p-lad commented Feb 7, 2019

mandel-macaque commented Jan 23, 2019 •

edited

Loading