Catch exception thrown by HttpConnection constructor and close connection

[martin-frydl]

It's a port from Mono where the issue is fixed (https://github.com/mono/mono/blob/master/mcs/class/System/System.Net/EndPointListener.cs#L117).

Fixes #32280

[stephentoub]

Thank you for porting the fix.

Two things:

1. Can you please add a corresponding test that would have crashed before the fix and now doesn't?
2. The managed code base for HttpListener was originally ported from Mono ~2 years ago. Have other fixes gone into that code that we should ensure are also ported over?

cc: @karelz

[martin-frydl]

1. Can you please add a corresponding test that would have crashed before the fix and now doesn't?

I can try but I'm not sure whether it will work correctly in tests. The original (old) behavior just crashed whole running process so it will probably crash and skip all following tests. I'm not sure how exactly the tests are run.

2. The managed code base for HttpListener was originally ported from Mono ~2 years ago.  Have other fixes gone into that code that we should ensure are also ported over?

I don't know. I'm not Mono developer, I just run our application on Mono before porting it to .NET Core now. I've checked the bugs we found in Mono implementation before and they all seem to be fixed in .NET Core version, except this one.

[stephentoub]

I can try but I'm not sure whether it will work correctly in tests. The original (old) behavior just crashed whole running process so it will probably crash and skip all following tests. I'm not sure how exactly the tests are run.

That's fine... with your fix it shouldn't crash, and if it does crash, that'll highlight something else that needs to be fixed.

I just run our application on Mono before porting it to .NET Core now. I've checked the bugs we found in Mono implementation before and they all seem to be fixed in .NET Core version, except this one.

Ok. Thank you for checking.

[karelz]

@stephentoub do you think it is a good idea to accept a test which uses Reflection on our code base?
I fear it might encourage more developers to use that pattern, which is not something we want IMO.
I would rather rely on fact that nothing is broken (no regression) + manual local test. Thoughts?

[stephentoub]

do you think it is a good idea to accept a test which uses Reflection on our code base?

In general, no. Why would we do that? I don't see why that's needed here.

[karelz]

@stephentoub the code that is changed here cannot be reached without tricking BCL via Reflection at this moment - see https://github.com/dotnet/corefx/issues/32280#issuecomment-421440910

[stephentoub]

the code that is changed here cannot be reached without tricking BCL via Reflection at this moment

I see. Then I think it's fine to just change it without adding a test. The code is just ported from Mono, so if it's currently effectively dead code in corefx, the top priority should be keeping it in sync with the Mono code, which as I understand it has this try/catch.

[karelz]

@martin-frydl if you can confirm that this change helps your scenario (with using Reflection) locally, we are ready to merge.
Thanks for porting it over!

[martin-frydl]

Yes, I can confirm this change fixes the problems.
Thank you for merging.

[karelz]

Thanks for confirmation and for the PR @martin-frydl!

[pavelchuchma]

Hi @karelz, please, is there a way to move this fix to 2.2 release? Martin did this fix to solve existing issue in our product going to be released in next weeks. Waiting several months for Core 3.0 release doesn't help us too much :-(

[karelz]

@pavelchuchma given that it is a change in unsupported code path and the demand is very low, it will not make it into servicing release of 2.1/2.2.

Your best options in the meantime are: Use 3.0 daily builds (without quality/support guarantees), or patch it yourself and use private builds off 2.2/2.1, or take the code and compile it into your app/library.