WebRequest.DefaultWebProxy: Set doesn't work without a previous Get #22798
Conversation
|
@arunjvs, It will cover your contributions to all .NET Foundation-managed open source projects. |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| namespace System.Net.Test.Common |
There was a problem hiding this comment.
This should not be the namespace for this class since this file is not in the Common\src directory. It should be the same namespace as the other test files in this folder.
| @@ -3,47 +3,19 @@ | |||
| // See the LICENSE file in the project root for more information. | |||
|
|
|||
| using System.Diagnostics; | |||
|
|
|||
| using System.Net.Test.Common; | |||
There was a problem hiding this comment.
Remove this once you fix up the DummyWebProxy.cs file.
| <Compile Include="HttpWebRequestTest.cs" /> | ||
| <Compile Include="HttpWebResponseTest.cs" /> | ||
| <Compile Include="RequestStreamTest.cs" /> | ||
| <Compile Include="WebRequestTest.cs" /> | ||
| <Compile Include="$(CommonTestPath)\System\PlatformDetection.cs"> |
There was a problem hiding this comment.
Why are you moving these lines? It isn't necessary as part of this change.
There was a problem hiding this comment.
Organizing the csproj itemgroups. Let me know if it's not desirable.
There was a problem hiding this comment.
Please remove this change. In general, you should keep your changes small and specific to the bug you are wishing to fix.
davidsh
changed the title
|
@arunjvs, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
| <Compile Include="HttpWebRequestTest.cs" /> | ||
| <Compile Include="HttpWebResponseTest.cs" /> | ||
| <Compile Include="RequestStreamTest.cs" /> | ||
| <Compile Include="WebRequestTest.cs" /> | ||
| <Compile Include="$(CommonTestPath)\System\PlatformDetection.cs"> |
There was a problem hiding this comment.
Please remove this change. In general, you should keep your changes small and specific to the bug you are wishing to fix.
| [Fact] | ||
| public void Select_Success() | ||
| { | ||
| RemoteInvoke(() => | ||
| { | ||
| var myProxy = new MyWebProxy(); | ||
| var myProxy = new DummyWebProxy(); |
There was a problem hiding this comment.
Does it need to be an instance of a new class like this? Could it not just be a new WebProxy()?
There was a problem hiding this comment.
WebProxy looks too generic a name for a dummy implementation. It sounds more like a base class for IWebProxy.
There was a problem hiding this comment.
I'm not talking about a dummy implementation. There's already a fairly generic WebProxy class that exists... I'm saying let's use it rather than adding a new file with a new dummy type... just use the type that already exists.
https://github.com/dotnet/corefx/blob/master/src/System.Net.WebProxy/src/System/Net/WebProxy.cs#L13
There was a problem hiding this comment.
Actually I realized it in indeed is. A separate dummy proxy still looks safe (keeping existing behavior in the tests). Let me know what you think.
There was a problem hiding this comment.
I don't understand. Sure, it's safe. But it's unnecessary code. Why an unnecessary code to the repo?
There was a problem hiding this comment.
Okay so should I keep GlobalProxySelectionTest as is with it's MyWebProxy or change it to WebProxy along with WebRequestTest?
There was a problem hiding this comment.
Revert the change to GlobalProxySelectionTest.
Then try the simple "WebProxy()" change for your new test.
There was a problem hiding this comment.
Done! Works with WebProxy().
|
Addressed comments and validated the test works with the change. |
…otnet#22798) * WebRequest.DefaultWebProxy: Set doesn't work without a previous Get
…otnet/corefx#22798) * WebRequest.DefaultWebProxy: Set doesn't work without a previous Get Commit migrated from dotnet/corefx@6acd74d
Iteration1: #22720.
Added test case as per comments in iteration1.
Unless we do an unnecessary Get() on WebRequest.DefaultWebProxy, any Set() done will not be honored.
This is especially a problem on Linux, where DefaultWebProxy throws PlatformNotSupportedException(). This in turn breaks libraries like WindowsAzure.Storage which fail unless the Get() is called first and then WebRequest.DefaultWebProxy is set to null.
This fix avoids the unnecessary Get() by setting the already initialized flag sent to the LazyInitializer.
@davidsh @stephentoub