Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
839063005:
Allow additional headers for WebSocket connect (Closed)
|
Created:
5 years, 11 months ago by Søren Gjesse Modified:
5 years, 11 months ago CC:
reviews_dartlang.org Target Ref:
refs/remotes/git-svn Visibility:
Public. |
DescriptionAllow additional headers for WebSocket connect
The WebSocket.connect now takes an Map of headers to add to the
initial handshake request.
BUG=http://dartbug.com/15855
R=kustermann@google.com, lrn@google.com
Committed: https://code.google.com/p/dart/source/detail?r=42870
Patch Set 1 #
Total comments: 20
Patch Set 2 : Changed from callback to passing headers as a Map #Patch Set 3 : Added Cache-Control header back #Patch Set 4 : Changed List to Iterable #
Total comments: 21
Patch Set 5 : Addressed review comments #Patch Set 6 : Rebased #
Messages
Total messages: 9 (0 generated)
I don't really like the way this is handled with a callback - not at all ! https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:151: Function onRequest}) => change the signature to this: onRequest(HttpClientRequest request) ? https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket_impl.dart File sdk/lib/io/websocket_impl.dart (left): https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket_impl.da... sdk/lib/io/websocket_impl.dart:803: ..set("Cache-Control", "no-cache") Why was this removed? https://codereview.chromium.org/839063005/diff/1/tests/standalone/io/web_sock... File tests/standalone/io/web_socket_test.dart (right): https://codereview.chromium.org/839063005/diff/1/tests/standalone/io/web_sock... tests/standalone/io/web_socket_test.dart:460: } [Since this file already imports async_helper, you could use it for making sure the callbacks get all called]
https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:134: * If [onRequest] is provided this function will be called with the I don't like the "onRequest" name. Its too vague. I also don't understand the HTTP-to-WS upgrade protocol, so I'm hard pressed to find a more suitable word. It seems that the user of this function needs to understand the underlying protocol in order to understand the parameter. That is: I read the documentation, and don't get any smarter. What is this "HTTP upgrade" you are talking about? Either specify as much of that protocol as necessary in the documentation, or find a way to reference it somewhere else. https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:137: * this if e.g. additional headers are required by the server. The Commas around "e.g.": "this if, e.g., additional"... https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:144: * upgrade: websocket Then make sure they aren't. If it's only those four, it should be simple to check and throw if the user tries to change them. You might need a wrapper object if the original request can't be locked down. https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:150: {List<String> protocols: const [], Why is "protocols" a List and not an Iterable? I could see the use for, e.g., a Set or the keys/values of a Map. https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket_impl.dart File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket_impl.da... sdk/lib/io/websocket_impl.dart:810: return result is Future Parentheses around (result is Future) expression (and generally around any non-trivial condition expression, where trivial really means "without spaces") https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket_impl.da... sdk/lib/io/websocket_impl.dart:815: } Rewrite as: if (onRequest != null) { var result = onRequest(request); if (result is Future) { return result.then((_) => request.close()); } } return request.close(); Is the code safe from memory leaks or similar if onRequest throws? https://codereview.chromium.org/839063005/diff/1/tests/standalone/io/web_sock... File tests/standalone/io/web_socket_test.dart (right): https://codereview.chromium.org/839063005/diff/1/tests/standalone/io/web_sock... tests/standalone/io/web_socket_test.dart:448: return useFuture ? new Future.value() : null; Consider actually delaying some action to just before the future completes, e.g., use the future: new Future.delayed(ms100).then((_) { request.headers.set('My-Header', 'my-value'); })
PTAL Using the callback was not a good choice. Changed to a Map of additional headers. Renamed subject as well. https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:134: * If [onRequest] is provided this function will be called with the On 2015/01/12 15:11:56, Lasse Reichstein Nielsen wrote: > I don't like the "onRequest" name. Its too vague. > I also don't understand the HTTP-to-WS upgrade protocol, so I'm hard pressed to > find a more suitable word. > > It seems that the user of this function needs to understand the underlying > protocol in order to understand the parameter. That is: I read the > documentation, and don't get any smarter. What is this "HTTP upgrade" you are > talking about? > > Either specify as much of that protocol as necessary in the documentation, or > find a way to reference it somewhere else. Changed to pass a map of additional headers https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:137: * this if e.g. additional headers are required by the server. The On 2015/01/12 15:11:56, Lasse Reichstein Nielsen wrote: > Commas around "e.g.": "this if, e.g., additional"... Rephrased comment. https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:144: * upgrade: websocket On 2015/01/12 15:11:56, Lasse Reichstein Nielsen wrote: > Then make sure they aren't. If it's only those four, it should be simple to > check and throw if the user tries to change them. You might need a wrapper > object if the original request can't be locked down. Changed to pass a map of additional headers, and documented that these headers are ignored. https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:150: {List<String> protocols: const [], On 2015/01/12 15:11:56, Lasse Reichstein Nielsen wrote: > Why is "protocols" a List and not an Iterable? > I could see the use for, e.g., a Set or the keys/values of a Map. No particular reason. Changed to Iterable. https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket.dart#ne... sdk/lib/io/websocket.dart:151: Function onRequest}) => On 2015/01/09 14:45:12, kustermann wrote: > change the signature to this: > > onRequest(HttpClientRequest request) > > ? Changed to not use callback. https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket_impl.dart File sdk/lib/io/websocket_impl.dart (left): https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket_impl.da... sdk/lib/io/websocket_impl.dart:803: ..set("Cache-Control", "no-cache") On 2015/01/09 14:45:12, kustermann wrote: > Why was this removed? I thought it was not needed as a proxy should never cache the response to an upgrade request. However that is of cause not always true, so added it back. https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket_impl.dart File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket_impl.da... sdk/lib/io/websocket_impl.dart:810: return result is Future On 2015/01/12 15:11:56, Lasse Reichstein Nielsen wrote: > Parentheses around (result is Future) expression (and generally around any > non-trivial condition expression, where trivial really means "without spaces") Dropped this code. https://codereview.chromium.org/839063005/diff/1/sdk/lib/io/websocket_impl.da... sdk/lib/io/websocket_impl.dart:815: } On 2015/01/12 15:11:56, Lasse Reichstein Nielsen wrote: > Rewrite as: > > if (onRequest != null) { > var result = onRequest(request); > if (result is Future) { > return result.then((_) => request.close()); > } > } > return request.close(); > > > Is the code safe from memory leaks or similar if onRequest throws? Dropped this code. https://codereview.chromium.org/839063005/diff/1/tests/standalone/io/web_sock... File tests/standalone/io/web_socket_test.dart (right): https://codereview.chromium.org/839063005/diff/1/tests/standalone/io/web_sock... tests/standalone/io/web_socket_test.dart:448: return useFuture ? new Future.value() : null; On 2015/01/12 15:11:56, Lasse Reichstein Nielsen wrote: > Consider actually delaying some action to just before the future completes, > e.g., use the future: > > new Future.delayed(ms100).then((_) { > request.headers.set('My-Header', 'my-value'); > }) > Removed the use of a callback. https://codereview.chromium.org/839063005/diff/1/tests/standalone/io/web_sock... tests/standalone/io/web_socket_test.dart:460: } On 2015/01/09 14:45:12, kustermann wrote: > [Since this file already imports async_helper, you could use it for making sure > the callbacks get all called] Done.
ping
lgtm https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:71: * request is not a valid web socket upgrade request a HTTP response a HTTP -> an HTTP https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:128: * Create a new web socket connection. The URL supplied in [url] I think "web socket" is usually written "WebSocket". Probably change it everywhere. https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:136: * header and potentially cookies. The key of the map are the header The key -> The keys. https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:137: * fields and the values are either String or List<String>. What is an "upgrade request"? Either say something about the underlying protocol or link to an external description. https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:139: * Even if [headers] is provided, there are a number of headers Drop the "even" part. Just go directly to the facts. https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:142: * `sec-websocket-key`, `upgrade` and `sec-websocket-protocol`. If Put the headers in alphabetical order. It looks random now. Maybe make it a bullet list? https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:143: * any of these are passed in the `headers` they will be ignored. add "map" after `headers`. Just "the headers" doesn't sound right. https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:146: {Iterable<String> protocols: const [], I'd use "null" as default, and then do the dispatch inside the function. No need to put the "const []" into the API documentation. https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket_imp... File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket_imp... sdk/lib/io/websocket_impl.dart:809: if (protocols.isNotEmpty) { protocols != null && ... (if removing the default value). Is the type still correct if "protocols" is just an Iterable, not a List?
lgtm https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket_imp... File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket_imp... sdk/lib/io/websocket_impl.dart:804: ..add(HttpHeaders.CONNECTION, "Upgrade") Why is this an .add() and not a .set(). If the user supplied a connection header as well, you have 2 of them, right? That's not what we want if I see this right. https://codereview.chromium.org/839063005/diff/60001/tests/standalone/io/web_... File tests/standalone/io/web_socket_test.dart (right): https://codereview.chromium.org/839063005/diff/60001/tests/standalone/io/web_... tests/standalone/io/web_socket_test.dart:445: Expect.equals('my-value-1, my-value-2', header[0]); I guess the []-operator on HttpHeaders is automatically merging the headers with ", "?
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 42870 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:128: * Create a new web socket connection. The URL supplied in [url] On 2015/01/14 10:49:21, Lasse Reichstein Nielsen wrote: > I think "web socket" is usually written "WebSocket". > Probably change it everywhere. Done. https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:136: * header and potentially cookies. The key of the map are the header On 2015/01/14 10:49:21, Lasse Reichstein Nielsen wrote: > The key -> The keys. Done. https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:137: * fields and the values are either String or List<String>. On 2015/01/14 10:49:21, Lasse Reichstein Nielsen wrote: > What is an "upgrade request"? Either say something about the underlying protocol > or link to an external description. Changed the initial upgrade request to setting up the connection https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:139: * Even if [headers] is provided, there are a number of headers On 2015/01/14 10:49:21, Lasse Reichstein Nielsen wrote: > Drop the "even" part. Just go directly to the facts. Done. https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:142: * `sec-websocket-key`, `upgrade` and `sec-websocket-protocol`. If On 2015/01/14 10:49:21, Lasse Reichstein Nielsen wrote: > Put the headers in alphabetical order. It looks random now. > Maybe make it a bullet list? Done. https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:143: * any of these are passed in the `headers` they will be ignored. On 2015/01/14 10:49:21, Lasse Reichstein Nielsen wrote: > add "map" after `headers`. Just "the headers" doesn't sound right. Done. https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket.dar... sdk/lib/io/websocket.dart:146: {Iterable<String> protocols: const [], On 2015/01/14 10:49:21, Lasse Reichstein Nielsen wrote: > I'd use "null" as default, and then do the dispatch inside the function. No need > to put the "const []" into the API documentation. Done. https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket_imp... File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket_imp... sdk/lib/io/websocket_impl.dart:804: ..add(HttpHeaders.CONNECTION, "Upgrade") On 2015/01/14 11:42:17, kustermann wrote: > Why is this an .add() and not a .set(). > > If the user supplied a connection header as well, you have 2 of them, right? > That's not what we want if I see this right. Good catch. https://codereview.chromium.org/839063005/diff/60001/sdk/lib/io/websocket_imp... sdk/lib/io/websocket_impl.dart:809: if (protocols.isNotEmpty) { On 2015/01/14 10:49:22, Lasse Reichstein Nielsen wrote: > protocols != null && ... > (if removing the default value). > > Is the type still correct if "protocols" is just an Iterable, not a List? Done. https://codereview.chromium.org/839063005/diff/60001/tests/standalone/io/web_... File tests/standalone/io/web_socket_test.dart (right): https://codereview.chromium.org/839063005/diff/60001/tests/standalone/io/web_... tests/standalone/io/web_socket_test.dart:445: Expect.equals('my-value-1, my-value-2', header[0]); On 2015/01/14 11:42:17, kustermann wrote: > I guess the []-operator on HttpHeaders is automatically merging the headers with > ", "? No, it is so stupid that [] returns one element for each header appearence. For My-Header: value-1 My-Header: value-2, value-3 headers['my-header'] returns ['value-1', 'value-2, value-3'] I am not going to say who the bright person designing this was. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
