[io/http] Add a HTTP Client parameter on WebSocket.connect to allow a custom HTTP Client for web socket connections. #46040

rnewquist · 2021-05-17T17:21:15Z

The WebSocket abstract class was changed to allow an optional parameter called customClient that takes in a HTTPClient and passes it to the WebSocket Implementation.
The WebSocket implementation takes the customClient, checks if its null, if its not null, it uses the customClient in place of the static HTTPClient that the WebSocket Implementation offers.
This custom client does not override the static HTTPClient, so all previous functionality remains the same when the customClient is not present.

TEST=testStaticClientUserAgentStaysTheSame() in web_socket_test.dart in standalone_2/standalone
TEST=new SecurityConfiguration(secure: true).runTests(); in web_socket_error_test.dart and web_socket_test.dart in standalone_2/standalone

Bug: #34284

rnewquist · 2021-05-17T17:22:05Z

@athomas it was such minor code, I just created a new PR.

rnewquist · 2021-05-17T21:46:41Z

This fixes #34284 by opening up the http client used in the web socket code, allowing users to add a custom Security Context and cert failure callback to the web socket. Currently, the only solution is to create a global http override, which isn't an ideal solution.

mit-mit · 2021-08-03T14:03:07Z

Code review is https://dart-review.googlesource.com/c/sdk/+/200262

a-siva · 2021-08-03T16:38:20Z

The signature of connect has been changed in the implementation but this PR does not seem to make the corresponding change in the 'abstract class WebSocket' code in websocket.dart.
Another question, the code could have set a userAgent using the 'set userAgent' method, now with the static variable _httpClient being changed to this new value, the getter userAgent would return whatever the new client has set. Not sure if this would be a problem, should the userAgent set previously be transferred to the new object ?

rnewquist · 2021-08-03T16:48:46Z

@a-siva
The abstract webSocket shouldn't need changing, it's only the connection part that needs to have the changes. The abstract webSocket has the appropriate connections, it's literally the httpClient being a private final that is preventing any sort of certificate pinning.
As for the second question, the user agent would get replaced if the new httpClient contains a user agent, however if it doesn't, the user agent gets set to what ever the private http client was set to.
_httpClient = customClient..userAgent ??= _httpClient.userAgent;
When creating the httpClient, there is no default userAgent, it gets set to null, so the code above would replace the userAgent with an existing user agent or another null value if the incoming http client contains a null userAgent.
I suppose another approach would be to allow the httpClient to have exposed getters and setters for the webSocket as well. I'm not sure if that would cause an issue though as I have not tested that, but I can verify, at least for my use case, this method does work for certificate pinning.

aam · 2021-08-03T17:36:14Z

sdk/lib/_http/websocket_impl.dart

+      HttpClient customClient}) {
+    //overriding _httpClient if user specifies a custom http client.
+    if (customClient != null) {
+      _httpClient = customClient..userAgent ??= _httpClient.userAgent;


I think overriding static _httpClient here will be confusing because it means that if you call connect twice with different customClients the first customClient will be overridden with second one. Currently it will result in userAgent for the fist connect reporting the value from customClient of the last connect.

I agree, however, with how its set up, it would require the httpClient to be tied to the webSocket connection itself (which I think it should be). However that problem exists in the current implementation does it not? So it doesn't solve that problem, but it does solve having a custom httpClient for all webSocket connections.

I agree, however, with how its set up, it would require the httpClient to be tied to the webSocket connection itself (which I think it should be).

Yeah, I think having httpClient as an instance field of _WebSocketImpl would be cleaner. It could default to static final HttpClient that can be used if user didn't provide customClient.

However that problem exists in the current implementation does it not?

Well, currently there is only one httpClient used by all web socket connections, so it being static is okay.

In that case, would it be best to create a separate ticket for instancing the httpClient with the webSocket? That said, I think that it would be much more clear to the user to have getters and setters for the static httpClient instead of having it passed as a method parameter. If you guys agree, i'll update this PR to reflect that.

If it wasn't clear by my proposal, having two separate tasks, one having the existing framework unchanged, but allowing the user to change the httpClient for all webSocket connections, and the other instancing the httpClient on a per webSocket connection basis. Since the first is literally making a private variable accessible, it shouldn't have much, if any, technical debt on the second task.

@aam I am not authorized to change the message in gerrit, so I updated the github message.

Thank you! Looking at how this ends up being copied into Gerrit by Copybara, I think [io/http] <One-liner... should be put into the subject of this PR(replacing exposed http client for web sockets subject) leaving only <Paragarph... etc in the description.
Also word TEST in Test=... lines should be all capital and there should be no space around equal sign.

You can see how we attempt to follow this convention in https://github.com/dart-lang/sdk/commits/main

@aam fixed, also added secure tests to web_socket_test.dart in both standalones.

FYI, if you start with a PR it's best to keep modifying the PR and just treat Gerrit as read-only (if you modify the review, that will likely break the automatic syncing from GitHub because of the conflicting edits).

@athomas I did not know that, sorry about that. I don't think I have any permission to edit the gerrit, so I "think" it should good, however I will keep that in mind for any future changes that I do.

athomas · 2021-09-09T16:24:49Z

Yes, only Googlers can trigger tryjobs at this time.

…

On Thu, 9 Sep 2021, 18:22 Alexander Aprelev, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sdk/lib/_http/websocket_impl.dart <#46040 (comment)>: > static Future<WebSocket> connect( String url, Iterable<String>? protocols, Map<String, dynamic>? headers, - {CompressionOptions compression = - CompressionOptions.compressionDefault}) { + {CompressionOptions compression = CompressionOptions.compressionDefault, + HttpClient customClient}) { + //overriding _httpClient if user specifies a custom http client. + if (customClient != null) { + _httpClient = customClient..userAgent ??= _httpClient.userAgent; @athomas <https://github.com/athomas> do you know if this is by design where PR authors are not able to trigger dry runs in Gerrit? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46040 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEYRJB6ZWBWWT3G6X3VGO2LUBDNLXANCNFSM45A4APRA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

aam · 2021-09-09T17:12:30Z

tests/standalone/io/web_socket_error_test.dart

+  Future<WebSocket> createClient(int port) => secure
+      ? WebSocket.connect('${secure ? "wss" : "ws"}://$HOST_NAME:$port/',
+          customClient: HttpClient(context: clientContext))
+      : WebSocket.connect('${secure ? "wss" : "ws"}://$HOST_NAME:$port/');


perhaps this can be simplified by passing null customClient if secure is false?

Future<WebSocket> createClient(int port) => WebSocket.connect('${secure ? "wss" : "ws"}://$HOST_NAME:$port/', customClient: secure ? HttpClient(context: clientContext): null);

completely agree, i wanted to keep the same logic as the server but didn't notice they were calling different functions. I have the customUserAgent there now for the added test.

aam · 2021-09-09T18:45:46Z

tests/standalone_2/io/web_socket_test.dart

-      WebSocket.connect('${secure ? "wss" : "ws"}://$HOST_NAME:$port/');
+  Future<WebSocket> createClient(int port, {String customUserAgent}) =>
+      WebSocket.connect('${secure ? "wss" : "ws"}://$HOST_NAME:$port/',
+          customClient: secure


Not sure if you are able to run the web_socket_test to troubleshoot the failure on Gerrit, hopefully you are.
Just to confirm - the issue here is that testShouldSetUserAgent when run with SecurityConfiguration secure:true fails due to the fact that global WebSocket.userAgent is not used in that setting - the connection is made using customClient with its userAgent.

I am not able to actually test it. However, WebSocket.userAgent comes from a final static, unless something else is setting the user agent, it shouldn't it never get changed? Would be a lot easier if I could debug it though.

You could try following https://github.com/dart-lang/sdk/wiki/Building to get the sdk built locally, test running.
Right, https://github.com/dart-lang/sdk/blob/main/tests/standalone/io/web_socket_test.dart#L550 from the test that fails is setting WebSocket.userAgent directly.

Took a while to get it up and running, but it was a simple fix. Because of how the tests are ran and because WebSocket.userAgent comes from a static final, testShouldSetUserAgent and testStaticClientUserAgentStaysTheSame were setting the userAgent to different values at roughly the same time. By just making sure that they are the same value, the static final variable doesn't get changed.

Thank you, it looks like there are more failures due to use of WebSocket.connect() when secure is true but without customClient that is needed.

I'm going to push a fix to the test to this PR if you don't mind @rnewquist.
It's basically about several places in the test where WebSocket.connect is invoked without provided customClient which is needed when running in secure configuration

@aam that would be perfect, I am a little busy this week, sorry about that.

aam · 2021-09-14T22:44:55Z

sdk/lib/_http/websocket_impl.dart

@@ -1022,7 +1022,9 @@ class _WebSocketImpl extends Stream with _ServiceObject implements WebSocket {
        path: uri.path,
        query: uri.query,
        fragment: uri.fragment);
-    return _httpClient.openUrl("GET", uri).then((request) {
+        // Use Custom HTTP Client first is it exists, else


I think this comment can be omitted as it just describes what is happening on the next line.
Ideally comments are answering the question "why?", rather than "what?", because normally code answers "what?" question already.

…dant comment

…e headers and user are passed when needed

aam · 2021-09-15T19:57:02Z

thank you @rnewquist once again!

a-siva · 2021-09-17T04:24:00Z

I noticed that the status files has a skip
io/web_socket_error_test: Skip # Flaky
for [ $compiler == dartkp && $runtime == dart_precompiled ]
The new tests added here do not run in precompiled mode, is that intended ?
It appears the fix for setting up the resources done in
https://dart-review.googlesource.com/c/sdk/+/213524
does not work for configurations that have a two step process,
see #47239

exposed http client

f1b570e

google-cla bot added the cla: yes label May 17, 2021

fixed error

d984abb

rnewquist changed the title ~~exposed http client~~ exposed http client for web sockets May 17, 2021

aam reviewed Aug 3, 2021

View reviewed changes

Ryan Newquist added 3 commits September 2, 2021 09:43

updated logic for custom client, added test

d7a770b

updated standalone 2 test

603577c

fixed error, fixed tests

c6a1913

Ryan Newquist added 2 commits September 9, 2021 12:45

fixed secure test

6224e41

updated tests to add secure option

c6a76bf

rnewquist changed the title ~~exposed http client for web sockets~~ [io/http] Add a HTTP Client parameter on WebSocket.connect to allow a custom HTTP Client for web socket connections. Sep 9, 2021

aam reviewed Sep 9, 2021

View reviewed changes

Ryan Newquist added 3 commits September 9, 2021 13:15

simplified logic

ea3e3ff

udpated tests

0bf5e80

updated error test

3330011

aam reviewed Sep 9, 2021

View reviewed changes

fixed tests

b362d27

aam reviewed Sep 14, 2021

View reviewed changes

aam added 3 commits September 14, 2021 16:06

Fix web_socket_test so it works in secure configuration. Remove redun…

0353608

…dant comment

Ensure optional parameters are nullable, remove unused imports, ensur…

0ed339b

…e headers and user are passed when needed

Run dart format

0de9f4e

dart-bot closed this in 2917c1c Sep 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[io/http] Add a HTTP Client parameter on WebSocket.connect to allow a custom HTTP Client for web socket connections. #46040

[io/http] Add a HTTP Client parameter on WebSocket.connect to allow a custom HTTP Client for web socket connections. #46040

rnewquist commented May 17, 2021 •

edited

Loading

rnewquist commented May 17, 2021

rnewquist commented May 17, 2021

mit-mit commented Aug 3, 2021

a-siva commented Aug 3, 2021

rnewquist commented Aug 3, 2021 •

edited

Loading

aam Aug 3, 2021

rnewquist Aug 3, 2021 •

edited

Loading

aam Aug 3, 2021

rnewquist Aug 3, 2021

rnewquist Aug 3, 2021

rnewquist Sep 9, 2021

aam Sep 9, 2021

rnewquist Sep 9, 2021

athomas Sep 9, 2021

rnewquist Sep 9, 2021

athomas commented Sep 9, 2021 via email

aam Sep 9, 2021 •

edited

Loading

rnewquist Sep 9, 2021 •

edited

Loading

aam Sep 9, 2021 •

edited

Loading

rnewquist Sep 9, 2021

aam Sep 9, 2021 •

edited

Loading

rnewquist Sep 9, 2021

aam Sep 9, 2021

aam Sep 14, 2021

rnewquist Sep 14, 2021

aam Sep 14, 2021

aam commented Sep 15, 2021

a-siva commented Sep 17, 2021

[io/http] Add a HTTP Client parameter on WebSocket.connect to allow a custom HTTP Client for web socket connections. #46040

[io/http] Add a HTTP Client parameter on WebSocket.connect to allow a custom HTTP Client for web socket connections. #46040

Conversation

rnewquist commented May 17, 2021 • edited Loading

rnewquist commented May 17, 2021

rnewquist commented May 17, 2021

mit-mit commented Aug 3, 2021

a-siva commented Aug 3, 2021

rnewquist commented Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

rnewquist Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athomas commented Sep 9, 2021 via email

aam Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

rnewquist Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

aam Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aam Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aam commented Sep 15, 2021

a-siva commented Sep 17, 2021

rnewquist commented May 17, 2021 •

edited

Loading

rnewquist commented Aug 3, 2021 •

edited

Loading

rnewquist Aug 3, 2021 •

edited

Loading

aam Sep 9, 2021 •

edited

Loading

rnewquist Sep 9, 2021 •

edited

Loading

aam Sep 9, 2021 •

edited

Loading

aam Sep 9, 2021 •

edited

Loading