Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Use ManagedWebSocket implementation everywhere except uap. #26429

Merged
merged 2 commits into from
Jan 23, 2018
Merged

Use ManagedWebSocket implementation everywhere except uap. #26429

merged 2 commits into from
Jan 23, 2018

Conversation

Priya91
Copy link
Contributor

@Priya91 Priya91 commented Jan 18, 2018 •
edited

davidsh
davidsh approved these changes Jan 18, 2018
View reviewed changes
Copy link
Contributor

@davidsh davidsh left a comment

Choose a reason for hiding this comment

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

LGTM. Please run outerloop tests before merge.

@Priya91
Copy link
Contributor Author

Priya91 commented Jan 18, 2018

@dotnet-bot test outerloop windows x64 debug build please
@dotnet-bot test outerloop linux x64 debug build please
@dotnet-bot test outerloop osx x64 debug build please

@CIPop
Copy link
Member

CIPop commented Jan 19, 2018

Thanks @Priya91 !

I remember we didn't have Proxy tests for WS (maybe that changed). It would be great if you could confirm both Proxy and ClientCertificates work.

@Priya91
Copy link
Contributor Author

Priya91 commented Jan 19, 2018

HttpListener uses websocket for upgrading connection to websocket protocol. Investigating the test failures.

@Priya91
Copy link
Contributor Author

Priya91 commented Jan 19, 2018

I remember we didn't have Proxy tests for WS (maybe that changed). It would be great if you could confirm both Proxy and ClientCertificates work.

Will verify.

stephentoub
stephentoub approved these changes Jan 19, 2018
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks, @Priya91!

(I was going to suggest not deleting the WinHttp implementation yet and just switching the default, so that we could relatively easy run new tests against the WinHttp implementation to validate compat, but even with deleting it we can easily run the tests against the netfx implementation, which is the main thing we want to be compatible with, so we should be good.)

@Priya91
Copy link
Contributor Author

Priya91 commented Jan 19, 2018

@dotnet-bot test outerloop windows x64 debug build please
@dotnet-bot test outerloop linux x64 debug build please
@dotnet-bot test outerloop osx x64 debug build please

@Priya91
Copy link
Contributor Author

Priya91 commented Jan 19, 2018

ClientCertificates and Proxy options is sent to the ManagedHandler from the ClientWebSocket. ManagedHandler supports client certificates, but not proxy yet. @wfurt is working on it currently so it should be supported in 2.1

@stephentoub
Copy link
Member

but not proxy yet

To be clear, it supports proxies, just not for https yet.

@CIPop
Copy link
Member

CIPop commented Jan 19, 2018

To be clear, it supports proxies, just not for https yet.

@stephentoub @wfurt Sorry but this actually made it less clear :). Can you please confirm if Proxy over HTTPS will be supported in 2.1?

@stephentoub
Copy link
Member

Will be, yes. Currently, no.

@karelz
Copy link
Member

karelz commented Jan 19, 2018 •
edited

@CIPop currently there is bunch of related issues:

  1. SSL proxy tunneling - #23136
  2. Support https proxy - #26460 [EDIT] Not supported even by .NET Framework
  3. Default proxy configuration - #23150
  4. Basic proxy authentication does not work - #26405

Can you please confirm you need both [1] and [2]?

@stephentoub
Copy link
Member

stephentoub commented Jan 19, 2018 •
edited

@karelz, how is (2) different from (1)? It looks like a dup. Or (2) means the proxy server itself is actually accessed via a secure connection?

@wfurt
Copy link
Member

wfurt commented Jan 19, 2018

I think it is this @stephentoub https://daniel.haxx.se/blog/2016/11/26/https-proxy-with-curl/
Tunneling use CONNECT method to upgrade connection to tls transport.

@CIPop
Copy link
Member

CIPop commented Jan 19, 2018

@karelz @stephentoub I was confused as well with my previous question and original request to support Proxy...
I don't believe we need proxy on our side at this time: our connections are only HTTPS and WSS. (2) as described by @wfurt seems to be experimental for Curl and lower level SOCKS4/5 proxies were never supported by .NET Framework.

@Priya91 Priya91 force-pushed the websocket branch 2 times, most recently from 393802a to b12c6b5 Compare January 20, 2018 00:26
@Priya91
Copy link
Contributor Author

Priya91 commented Jan 23, 2018

Test failures are known issues #26531 #25676

@Priya91 Priya91 merged commit bb00131 into dotnet:master Jan 23, 2018
@Priya91 Priya91 deleted the websocket branch January 23, 2018 04:55
@karelz karelz added this to the 2.1.0 milestone Feb 4, 2018
@caesar-chen caesar-chen mentioned this pull request Feb 15, 2018
Merged
@kblok kblok mentioned this pull request May 13, 2018
Merged
@rmkerr rmkerr mentioned this pull request Jan 31, 2020
Closed
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@Priya91
Merge pull request dotnet/corefx#26429 from Priya91/websocket
5e7b438 
Use ManagedWebSocket implementation everywhere except uap.

Commit migrated from dotnet/corefx@bb00131
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@davidsh davidsh davidsh approved these changes

Assignees

@Priya91 Priya91

Labels
area-System.Net
Projects
None yet
Milestone
2.1.0
6 participants
@Priya91 @CIPop @stephentoub @karelz @wfurt @davidsh