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.

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

Avoid issuing connection attempts for already cancelled requests #66992

Merged

Conversation

MihaZupan
Copy link
Member

Fixes #66990

When checking whether to inject a new connection, check that a non-cancelled request exists, dropping any cancelled entries from the queue.

@MihaZupan MihaZupan added this to the 7.0.0 milestone Mar 22, 2022
@MihaZupan MihaZupan requested a review from a team March 22, 2022 15:10
@ghost ghost assigned MihaZupan Mar 22, 2022
@ghost
Copy link

ghost commented Mar 22, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #66990

When checking whether to inject a new connection, check that a non-cancelled request exists, dropping any cancelled entries from the queue.

Author: MihaZupan
Assignees: -
Labels:

area-System.Net.Http

Milestone: 7.0.0

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@danmoseley
Copy link
Member

Failure on WASM

[20:23:18] info: Initializing.....
[20:23:18] info: Discovering: System.Net.Http.Functional.Tests.dll (method display = ClassAndMethod, method display options = None)
[20:23:21] info: Discovered:  System.Net.Http.Functional.Tests.dll (found 433 of 1613 test cases)
[20:23:21] info: Using random seed for test cases: 1510065757
[20:23:21] info: Using random seed for collections: 1510065757
[20:23:21] info: Starting:    System.Net.Http.Functional.Tests.dll
[20:23:26] fail: [out of order message from the browser]: http://127.0.0.1:40179/Echo.ashx?auth=basic&user=testuser&password=password - Failed to load resource: the server responded with a status of 401 (Unauthorized)
[20:23:26] fail: [out of order message from the browser]: https://127.0.0.1:33455/Echo.ashx?auth=basic&user=testuser&password=password - Failed to load resource: the server responded with a status of 401 ()
[20:23:26] fail: [out of order message from the browser]: https://127.0.0.1:33455/Echo.ashx?auth=basic&user=testuser&password=password - Failed to load resource: the server responded with a status of 401 ()
[20:23:29] fail: [out of order message from the browser]: http://127.0.0.1:43681/ - Failed to load resource: net::ERR_FAILED
[20:23:30] fail: [FAIL] System.Net.Http.Functional.Tests.SocketsHttpHandler_HttpClientHandler_Http11_Cancellation_Test.RequestsCanceled_NoConnectionAttemptForCanceledRequests
[20:23:30] info: System.PlatformNotSupportedException : Operation is not supported on this platform.
[20:23:30] info:    at System.Net.Http.BrowserHttpHandler.set_MaxConnectionsPerServer(Int32 value)
[20:23:30] info:    at System.Net.Http.HttpClientHandler.set_MaxConnectionsPerServer(Int32 value)
[20:23:30] info:    at System.Net.Http.Functional.Tests.SocketsHttpHandler_Cancellation_Test.RequestsCanceled_NoConnectionAttemptForCanceledRequests()
[20:23:30] info: --- End of stack trace from previous location ---
[20:23:33] info: http://127.0.0.1:40179/index.html?arg=--setenv%3dDOTNET_TEST_WEBSOCKETHOST%3d127.0.0.1%3a40179&arg=--setenv%3dDOTNET_TEST_HTTPHOST%3d127.0.0.1%3a40179&arg=--setenv%3dDOTNET_TEST_REMOTE_LOOP_HOST%3d127.0.0.1%3a40179&arg=--setenv%3dDOTNET_TEST_SECUREWEBSOCKETHOST%3d127.0.0.1%3a33455&arg=--setenv%3dDOTNET_TEST_SECUREHTTPHOST%3d127.0.0.1%3a33455&arg=--setenv%3dDOTNET_TEST_HTTP2HOST%3d127.0.0.1%3a33455&arg=--run&arg=WasmTestRunner.dll&arg=System.Net.Http.Functional.Tests.dll&arg=-notrait&arg=category%3dIgnoreForCI&arg=-notrait&arg=category%3dOuterLoop&arg=-notrait&arg=category%3dfailing - "Authorization" will not be covered by the wildcard symbol (*)in CORS "Access-Control-Allow-Headers" handling.
[20:23:35] info: Finished:    System.Net.Http.Functional.Tests.dll
[20:23:35] info: 
[20:23:35] info: === TEST EXECUTION SUMMARY ===

@MihaZupan
Copy link
Member Author

Are OuterLoop tests not ran on WASM? All other tests in this test class should blow up the same way.

@danmoseley
Copy link
Member

Did you request outer loop? I don't see it above?

@MihaZupan
Copy link
Member Author

MihaZupan commented Mar 23, 2022

I haven't.
I mean that this class is not guarding against SocketsHttpHandler.IsSupported in general, so all the existing tests should always blow up if you try to run them on WASM (but they were all outerloop until now).
I would have expected we would see failures any time we ran OuterLoop in the past - unless OuterLoop never runs on WASM?

@danmoseley
Copy link
Member

Ah - I don't know, but I don't see 'runtime-wasm' listed here

- name: includeWindowsOuterloop
value: ${{ or(endsWith(variables['Build.DefinitionName'], 'windows'), endsWith(variables['Build.DefinitionName'], 'outerloop')) }}
- name: includeLinuxOuterloop
value: ${{ or(endsWith(variables['Build.DefinitionName'], 'linux'), endsWith(variables['Build.DefinitionName'], 'outerloop')) }}
- name: includeOsxOuterloop
value: ${{ or(endsWith(variables['Build.DefinitionName'], 'osx'), endsWith(variables['Build.DefinitionName'], 'outerloop')) }}

although it is here

value: ${{ ne(variables['Build.DefinitionName'], 'runtime-wasm') }}

@lewing @steveisok do you have context on whether outerloop has ever run on wasm?

@MihaZupan MihaZupan merged commit 8c7120e into dotnet:main Mar 24, 2022
MihaZupan added a commit to MihaZupan/runtime that referenced this pull request Mar 28, 2022
…net#66992)

* Avoid issuing connection attempts for already canceled requests

* Cancelled => Canceled

* Guard SocketsHttpHandler tests under SocketsHttpHandler.IsSupported
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
…net#66992)

* Avoid issuing connection attempts for already canceled requests

* Cancelled => Canceled

* Guard SocketsHttpHandler tests under SocketsHttpHandler.IsSupported
carlossanlop pushed a commit that referenced this pull request Apr 13, 2022
) (#67226)

* Avoid issuing connection attempts for already canceled requests

* Cancelled => Canceled

* Guard SocketsHttpHandler tests under SocketsHttpHandler.IsSupported
@ghost ghost locked as resolved and limited conversation to collaborators Apr 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpConnectionPool attempts to create connections for pre-canceled requests
5 participants