Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

[release/3.1] use background thread to wait for address changes on OSX (#41768)#41960

Merged
danmoseley merged 1 commit intodotnet:release/3.1from
danmoseley:portBGthread2
Oct 21, 2019
Merged

[release/3.1] use background thread to wait for address changes on OSX (#41768)#41960
danmoseley merged 1 commit intodotnet:release/3.1from
danmoseley:portBGthread2

Conversation

@danmoseley
Copy link
Member

@danmoseley danmoseley commented Oct 21, 2019

Description

Use background thread to wait for address changes on OSX to avoid hang after:

System.Net.NetworkInformation.NetworkChange.NetworkAddressChanged += (sender, e) => {}

ports #41768
fixes #41740

Customer Impact:

Requested by @davidfowl : "We found this when we noticed that applications which use AppInsights don't close when they would normally exit on a Mac. This isn't noticed normally because the normal workflow for AspNetCore apps is that when you want the server to exit you send Ctrl-C."

Regression?

No.

Risk

Very low. The fix is just changing a thread to Background, and the only effect of that is to prevent the runtime joining on it before terminating.

Tests run / added

Test added.

@wfurt @danmoseley
use background thread to wait for address changes on OSX (dotnet#41768)
5044b6d 
* use background thread to wait for address changes on OSX

* remove RemoteExecutor

* update comment
@danmoseley danmoseley requested a review from wfurt October 21, 2019 18:51
@danmoseley danmoseley added this to the 3.1 milestone Oct 21, 2019
@danmoseley
Copy link
Member Author

danmoseley commented Oct 21, 2019

@stephentoub I have a conflict of interest. Do you support this backport?

@stephentoub
Copy link
Member

stephentoub commented Oct 21, 2019

Yeah, it's reasonable. Thanks.

wfurt
wfurt approved these changes Oct 21, 2019
View reviewed changes
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM.

@danmoseley danmoseley added Servicing-approved Approved for servicing release auto-merge Automatically merge PR once CI passes. labels Oct 21, 2019
@ghost
Copy link

ghost commented Oct 21, 2019

Hello @danmosemsft!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@mkArtakMSFT
Copy link

mkArtakMSFT commented Oct 21, 2019

Thanks @danmosemsft!

@danmoseley
Copy link
Member Author

danmoseley commented Oct 21, 2019

Failure was

Assert.Throws() Failure\r\nExpected: typeof(System.PlatformNotSupportedException)\r\nActual: (No exception was thrown)

Stack trace
   at System.Tests.EnvironmentTests.WorkingSet_Valid_Uap() in /_/src/System.Runtime.Extensions/tests/System/EnvironmentTests.cs:line 192

This is clearly not related. @ViktorHofer do you expect the UAP leg to work normally in release/3.1, unaffected by your recent changes?

merging.

@danmoseley danmoseley merged commit f36f5de into dotnet:release/3.1 Oct 21, 2019
@danmoseley danmoseley deleted the portBGthread2 branch October 21, 2019 21:44
@danmoseley danmoseley changed the title use background thread to wait for address changes on OSX (#41768) [release/3.1] use background thread to wait for address changes on OSX (#41768) Nov 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@wfurt wfurt wfurt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net auto-merge Automatically merge PR once CI passes. Servicing-approved Approved for servicing release

Projects

None yet

Milestone

3.1

Development

Successfully merging this pull request may close these issues.

5 participants

@danmoseley @stephentoub @mkArtakMSFT @wfurt @Dotnet-GitSync-Bot