Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1390353005:
Web Socket compression - take two (Closed)
|
Created:
5 years, 2 months ago by butlermatt Modified:
5 years, 1 month ago CC:
reviews_dartlang.org Base URL:
https://github.com/dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionWeb Socket compression - take two
Resolve issues with typed data tests and compression.
Uses HeaderValues to parse permessage_deflate option.
Includes changes from https://codereview.chromium.org/1208473005/
BUG=https://github.com/dart-lang/sdk/issues/14441
R=sgjesse@google.com
Committed: https://github.com/dart-lang/sdk/commit/59bb84127b63a7516a32a32eae6a6839420709db
Reverted: https://github.com/dart-lang/sdk/commit/d1e01e205a615257ba9391cb531ad8b053af1fed
Additional fixes for failing tests.
Committed: https://github.com/dart-lang/sdk/commit/95bb2159d9fb7f44d3bbc99932a57173e1b464aa
Patch Set 1 #
Total comments: 53
Patch Set 2 : Add custom compression header test #
Total comments: 14
Patch Set 3 : Address comments before rebase #Patch Set 4 : Rebase update #Patch Set 5 : Fix issue with invalid message not throwing #Patch Set 6 : Add valueSeparator param to HeaderValue.parse #Patch Set 7 : Rebase update #Patch Set 8 : Fix observatory issues #
Messages
Total messages: 18 (4 generated)
butler.matthew@gmail.com changed reviewers: + kaendfinger@gmail.com, kustermann@google.com, sgjesse@google.com
Continuation of web socket compression originally started by Kenneth. I've added him as a reviewer in addition to the original reviewers. I expect there may still be additional work to get this landed but believe I addressed most (all?) comments from the last patch set of the original CL.
alexandre.ardhuin@gmail.com changed reviewers: + alexandre.ardhuin@gmail.com
https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.dart File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:309: } You can remove this `if` by either : - `_eventSink.add(const Utf8Decoder().convert(bytes));` - `static final Utf8Decoder _utfDecoder = new Utf8Decoder();` as "class variable that’s declared as final is initialized the first time it’s used" (https://www.dartlang.org/docs/dart-up-and-running/ch02.html#final-and-const) https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:660: } You can remove this `if` with `static final Utf8Encoder _utfEncoder = new Utf8Encoder();` as "class variable that’s declared as final is initialized the first time it’s used" (https://www.dartlang.org/docs/dart-up-and-running/ch02.html#final-and-const)
Thanks for the updated version. With the comments addressed it is almost ready to be landed. The only place I think some additional work is needed on the parsing of 'Sec-WebSocket-Extensions', with some tests. The unit-testing of the dart:io private classes is not that simple. In tests/standalone/io/web_socket_test.dart you can see a way of sending negoation headers manually. Another - maybe better - option is to look at tests/standalone/io/web_socket_protocol_processor_test.dart, where we do test private classes by faking the dart:io library. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:75: this.clientMaxWindowBits, Assign default value _WebSocketImpl.DEFAULT_WINDOW_BITS here? https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:86: if (requested.parameters["server_max_window_bits"] != null) { Please make the values "server_no_context_takeover" , "server_max_window_bits", "client_max_window_bits" and "client_no_context_takeover" constants: const String clientNoContextTakeover = "client_no_context_takeover"; ... probably somewhere non-public. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:87: var part = requested.parameters["server_max_window_bits"]; Move this 'var part ...' up before the if, and test 'part' in the 'if' to avoid looking up the same value twice. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:89: ? int.parse(part, According to https://tools.ietf.org/html/draft-ietf-hybi-permessage-compression-27 the value must not have leading zeros. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:90: onError: (source) => _WebSocketImpl.DEFAULT_WINDOW_BITS) Please indent with 4 - or all the way to align with part if possible. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:193: * protocols. Please update the dartdoc comment here with information on the new compression argument. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:211: * protocols. ditto. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:309: * act as the server and will not mask its messages. ditto. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.dart File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:114: var rsv = (byte & (RSV1 | RSV2 | RSV3)) >> 4; I don't think this temp is needed (see below). https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:116: if (rsv != 0 && rsv != 4) { Just check ((byte & (RSV2 | RSV3)) != 0) https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:117: // The RSV1, RSV2, RSV3 bits must be all zero. This comment is no longer true. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:121: if (rsv == 4) { 4 -> RSV1 with no shift above. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:308: _utfDecoder = new Utf8Decoder(); Why not just use UTF8.decode? https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:533: if (clientMaxWindowBits == null) { Just add default values in the argument list? https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:593: var buffer = new Uint8List(msg.length); Avoid new buffer if msg is Uint8List. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:596: for(var i = 0; i < msg.length; i++) { Space after "for". https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:724: var rsv = 0; Use the RSVx constants. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:730: var hoc = (1 << 7) | (rsv % 8) << 4 | (opcode % 128); How about var hoc = FIN | (compressed ? RSV1 : 0) | (opcode & OPCODE); and no rsv temp? https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:994: if(compression.enabled) { Space after "if" https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:1052: extensionHeader.split(", ").map((it) => it.split("; ")); This seems fragile. I don't think the specification requires space after , or ;. Also how about quoted values containing , or ; inside quotes? E.g.: Sec-WebSocket-Extensions: my_ext; x="per_message_deflate; server_no_context_takeover; server_max_window_bits=7", per_message_deflate; server_max_window_bits=15 I think a real parser is required here. See my comment on this in the previous CL. You already did some of the suggested changes to http_header.dart. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:1061: var o = opts.firstWhere((x) => x.startsWith("${type}_max_window_bits="), This is also fragile. Doesn't the spec allow for whitespace on both sides of the '='? https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:1070: o = int.parse(o); According to the spec the value can be quoted. https://codereview.chromium.org/1390353005/diff/1/tests/standalone/io/web_soc... File tests/standalone/io/web_socket_compression_test.dart (right): https://codereview.chromium.org/1390353005/diff/1/tests/standalone/io/web_soc... tests/standalone/io/web_socket_compression_test.dart:51: WebSocketTransformer.upgrade(request, compression: options).then((webSocket) { Long line. https://codereview.chromium.org/1390353005/diff/1/tests/standalone/io/web_soc... tests/standalone/io/web_socket_compression_test.dart:79: testCompressionSupport(true, true); Please add tests where the client wants to use compression, but the server does not support it.
I believe I've addressed the comments. I also added a test to account for custom compression headers including quoted values and space padding around an = sign. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:75: this.clientMaxWindowBits, On 2015/10/19 16:53:20, Søren Gjesse wrote: > Assign default value _WebSocketImpl.DEFAULT_WINDOW_BITS here? Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:86: if (requested.parameters["server_max_window_bits"] != null) { On 2015/10/19 16:53:19, Søren Gjesse wrote: > Please make the values "server_no_context_takeover" , "server_max_window_bits", > "client_max_window_bits" and "client_no_context_takeover" constants: > > const String clientNoContextTakeover = "client_no_context_takeover"; > ... > > probably somewhere non-public. Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:87: var part = requested.parameters["server_max_window_bits"]; On 2015/10/19 16:53:19, Søren Gjesse wrote: > Move this 'var part ...' up before the if, and test 'part' in the 'if' to avoid > looking up the same value twice. Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:89: ? int.parse(part, On 2015/10/19 16:53:19, Søren Gjesse wrote: > According to > https://tools.ietf.org/html/draft-ietf-hybi-permessage-compression-27 the value > must not have leading zeros. Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:90: onError: (source) => _WebSocketImpl.DEFAULT_WINDOW_BITS) On 2015/10/19 16:53:20, Søren Gjesse wrote: > Please indent with 4 - or all the way to align with part if possible. Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:193: * protocols. On 2015/10/19 16:53:20, Søren Gjesse wrote: > Please update the dartdoc comment here with information on the new compression > argument. Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:211: * protocols. On 2015/10/19 16:53:19, Søren Gjesse wrote: > ditto. Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:309: * act as the server and will not mask its messages. On 2015/10/19 16:53:20, Søren Gjesse wrote: > ditto. Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.dart File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:114: var rsv = (byte & (RSV1 | RSV2 | RSV3)) >> 4; On 2015/10/19 16:53:20, Søren Gjesse wrote: > I don't think this temp is needed (see below). Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:116: if (rsv != 0 && rsv != 4) { On 2015/10/19 16:53:20, Søren Gjesse wrote: > Just check ((byte & (RSV2 | RSV3)) != 0) Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:117: // The RSV1, RSV2, RSV3 bits must be all zero. On 2015/10/19 16:53:20, Søren Gjesse wrote: > This comment is no longer true. Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:121: if (rsv == 4) { On 2015/10/19 16:53:20, Søren Gjesse wrote: > 4 -> RSV1 with no shift above. Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:308: _utfDecoder = new Utf8Decoder(); On 2015/10/19 16:53:20, Søren Gjesse wrote: > Why not just use UTF8.decode? Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:309: } On 2015/10/13 20:22:57, alexandre.ardhuin wrote: > You can remove this `if` by either : > - `_eventSink.add(const Utf8Decoder().convert(bytes));` > - `static final Utf8Decoder _utfDecoder = new Utf8Decoder();` as "class variable > that’s declared as final is initialized the first time it’s used" > (https://www.dartlang.org/docs/dart-up-and-running/ch02.html#final-and-const) Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:533: if (clientMaxWindowBits == null) { On 2015/10/19 16:53:20, Søren Gjesse wrote: > Just add default values in the argument list? Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:593: var buffer = new Uint8List(msg.length); On 2015/10/19 16:53:20, Søren Gjesse wrote: > Avoid new buffer if msg is Uint8List. Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:596: for(var i = 0; i < msg.length; i++) { On 2015/10/19 16:53:20, Søren Gjesse wrote: > Space after "for". Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:660: } On 2015/10/13 20:22:57, alexandre.ardhuin wrote: > You can remove this `if` with `static final Utf8Encoder _utfEncoder = new > Utf8Encoder();` as "class variable > that’s declared as final is initialized the first time it’s used" > (https://www.dartlang.org/docs/dart-up-and-running/ch02.html#final-and-const) Converted to UTF8.encode to match previous comment regarding UTF8.decode https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:724: var rsv = 0; On 2015/10/19 16:53:20, Søren Gjesse wrote: > Use the RSVx constants. Acknowledged. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:730: var hoc = (1 << 7) | (rsv % 8) << 4 | (opcode % 128); On 2015/10/19 16:53:20, Søren Gjesse wrote: > How about > > var hoc = FIN | (compressed ? RSV1 : 0) | (opcode & OPCODE); > > and no rsv temp? Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:994: if(compression.enabled) { On 2015/10/19 16:53:20, Søren Gjesse wrote: > Space after "if" Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:1052: extensionHeader.split(", ").map((it) => it.split("; ")); On 2015/10/19 16:53:20, Søren Gjesse wrote: > This seems fragile. I don't think the specification requires space after , or ;. > Also how about quoted values containing , or ; inside quotes? > > E.g.: > > Sec-WebSocket-Extensions: my_ext; x="per_message_deflate; > server_no_context_takeover; server_max_window_bits=7", per_message_deflate; > server_max_window_bits=15 > > I think a real parser is required here. > > See my comment on this in the previous CL. You already did some of the suggested > changes to http_header.dart. Done. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:1061: var o = opts.firstWhere((x) => x.startsWith("${type}_max_window_bits="), On 2015/10/19 16:53:20, Søren Gjesse wrote: > This is also fragile. Doesn't the spec allow for whitespace on both sides of the > '='? Acknowledged. https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:1070: o = int.parse(o); On 2015/10/19 16:53:20, Søren Gjesse wrote: > According to the spec the value can be quoted. Acknowledged. https://codereview.chromium.org/1390353005/diff/1/tests/standalone/io/web_soc... File tests/standalone/io/web_socket_compression_test.dart (right): https://codereview.chromium.org/1390353005/diff/1/tests/standalone/io/web_soc... tests/standalone/io/web_socket_compression_test.dart:51: WebSocketTransformer.upgrade(request, compression: options).then((webSocket) { On 2015/10/19 16:53:20, Søren Gjesse wrote: > Long line. Done. https://codereview.chromium.org/1390353005/diff/1/tests/standalone/io/web_soc... tests/standalone/io/web_socket_compression_test.dart:79: testCompressionSupport(true, true); On 2015/10/19 16:53:20, Søren Gjesse wrote: > Please add tests where the client wants to use compression, but the server does > not support it. Done.
https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.dart File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/1390353005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:308: _utfDecoder = new Utf8Decoder(); On 2015/10/22 19:36:09, butlermatt wrote: > On 2015/10/19 16:53:20, Søren Gjesse wrote: > > Why not just use UTF8.decode? > > Done. Unlike `UTF8.encode` that uses const, `UTF8.decode` creates a new instance of `Utf8Decoder` everytime `UTF8.decode` is call (see https://github.com/dart-lang/sdk/blob/master/sdk/lib/convert/utf.dart#L69-L72). That's why I think `const Utf8Decoder().convert(bytes)` is better.
LGTM, with the additional comments addressed. Please also rebase to head (preferable as a separate Patch Set - I actually don't think there has been any changes to the files you have worked on) I can then try to land it. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/http_headers... File sdk/lib/io/http_headers.dart (right): https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/http_headers... sdk/lib/io/http_headers.dart:751: skipWS(); I think you should add if (done()) return; here instead of in maybeExpect. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/http_headers... sdk/lib/io/http_headers.dart:753: skipWS(); ditto add if (done()) return; here instead of in parseParameterValue. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket.da... sdk/lib/io/websocket.dart:89: if(part.length >= 2 && part.startsWith('0')) { nit: Space after if. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket.da... sdk/lib/io/websocket.dart:89: if(part.length >= 2 && part.startsWith('0')) { I am not sure what is the right thing here. According to the spec leading zeros are not allowed, so maybe just terminate the connection here with an error instead of using the default. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket_im... File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket_im... sdk/lib/io/websocket_impl.dart:586: if(msg is! Uint8List) { nit: Space after if. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket_im... sdk/lib/io/websocket_impl.dart:587: buffer = new Uint8List(msg.length); New Uint8List.fromList(msg) will be simpler here. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket_im... sdk/lib/io/websocket_impl.dart:1051: var o = hv.parameters['${type}_max_window_bits']; Please use the constants here and switch on the type - maybe change it to a bool.
Addressed comments. Added rebase as final patch set. note for HttpHeaders, the if (done()) checks still needed to add the key with the null value to the parameters, so that parameters.containsKey(..) checks would still work. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/http_headers... File sdk/lib/io/http_headers.dart (right): https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/http_headers... sdk/lib/io/http_headers.dart:751: skipWS(); On 2015/10/23 15:09:59, Søren Gjesse wrote: > I think you should add > > if (done()) return; > > here instead of in maybeExpect. Done. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/http_headers... sdk/lib/io/http_headers.dart:753: skipWS(); On 2015/10/23 15:09:59, Søren Gjesse wrote: > ditto add > > if (done()) return; > > here instead of in parseParameterValue. Done. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket.da... sdk/lib/io/websocket.dart:89: if(part.length >= 2 && part.startsWith('0')) { On 2015/10/23 15:09:59, Søren Gjesse wrote: > nit: Space after if. Done. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket.da... sdk/lib/io/websocket.dart:89: if(part.length >= 2 && part.startsWith('0')) { On 2015/10/23 15:09:59, Søren Gjesse wrote: > I am not sure what is the right thing here. According to the spec leading zeros > are not allowed, so maybe just terminate the connection here with an error > instead of using the default. Done. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket_im... File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket_im... sdk/lib/io/websocket_impl.dart:586: if(msg is! Uint8List) { On 2015/10/23 15:09:59, Søren Gjesse wrote: > nit: Space after if. Done. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket_im... sdk/lib/io/websocket_impl.dart:587: buffer = new Uint8List(msg.length); On 2015/10/23 15:09:59, Søren Gjesse wrote: > New Uint8List.fromList(msg) will be simpler here. Done. https://codereview.chromium.org/1390353005/diff/20001/sdk/lib/io/websocket_im... sdk/lib/io/websocket_impl.dart:1051: var o = hv.parameters['${type}_max_window_bits']; On 2015/10/23 15:09:59, Søren Gjesse wrote: > Please use the constants here and switch on the type - maybe change it to a > bool. Done. I changed the call to use the constants here instead. I'm not sure I understand change what to a bool?
Sorry, there's still a final bug with the compression allowing Uint8List > 256 go through. Once I track that down it should be ready again. This may be related to the concern I raised on the zlib compression not validating that values are < 256 and just swallowing the difference
My apologies. I've corrected the issue with the type data not throwing properly when values are outside of range of Uint8. I created an associated bug as well: https://github.com/dart-lang/sdk/issues/24708 That said, for some reason I still have not been able to determine using: msg.any((el) => el < 0 || el > 255) would not cause the error to throw but using a for loop to manually iterate over elements would. (in my initial previous one I also had a bug in that it was checking the buffer after it had been converted but even using the correct check was still not throwing as expected. In anycase, using the for loop does work properly and throws the errors to satisfy the tests (web_socket_typed_data in particular)
I tried to land the change, but there is still a few small
problems that I did not discover in the review. This results in
the following two tests timing out:
standalone/io/http_auth_digest_test
standalone/io/http_proxy_advanced_test
I looked into the issue, and it turns out the that this is
caused by the changes to the header value parsing. The tests
that fail are using header values like this:
Digest, realm="test", nonce="12345678", algorithm=MD5, domain="/digest/",
qop="auth"
and the parsing is done like this:
_HeaderValue header =
_HeaderValue.parse(challenge[0], parameterSeparator: ",");
With the changes where ',' is used to explicitly terminate parsing
this breaks.
Either you should use a separate parser for the
Sec-WebSocket-Extensions header value or perhaps add an
additional optional argument - valueSeparator - to
HeaderValue.parse and for now just stop after one
value. valueSeparator should default to null for the current
behaviour.
There was also a syntax error in the file sdk/lib/io/http_headers.dart.
If you run this test:
./tools/test.py -m release dart2js/analyze_api_test
dart2js will analyze all library code.
On 2015/10/28 11:52:47, Søren Gjesse wrote: > I tried to land the change, but there is still a few small > problems that I did not discover in the review. This results in > the following two tests timing out: > > standalone/io/http_auth_digest_test > standalone/io/http_proxy_advanced_test > > I looked into the issue, and it turns out the that this is > caused by the changes to the header value parsing. The tests > that fail are using header values like this: > > Digest, realm="test", nonce="12345678", algorithm=MD5, domain="/digest/", > qop="auth" > > and the parsing is done like this: > > _HeaderValue header = > _HeaderValue.parse(challenge[0], parameterSeparator: ","); > > With the changes where ',' is used to explicitly terminate parsing > this breaks. > > Either you should use a separate parser for the > Sec-WebSocket-Extensions header value or perhaps add an > additional optional argument - valueSeparator - to > HeaderValue.parse and for now just stop after one > value. valueSeparator should default to null for the current > behaviour. > > There was also a syntax error in the file sdk/lib/io/http_headers.dart. > If you run this test: > > ./tools/test.py -m release dart2js/analyze_api_test > > dart2js will analyze all library code. Thanks Søren, Sorry about that oversight on my part. I've addressed those issues, choosing to add the valueSeparator parameter to HeaderValue.parse I've run through most tests (those in standalone/io plus the analyzer again) and everything is coming through cleanly now. I've also added a separate patch with another rebase to be on the safe side. Near as I can tell, we should be good to go now. Matt
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as 59bb84127b63a7516a32a32eae6a6839420709db (presubmit successful).
Message was sent while issue was closed.
Additional fixes for the observatory tests. I've also tested these patches on Mojo and Flutter engine and they both continue to pass their respective tests.
Message was sent while issue was closed.
Description was changed from ========== Web Socket compression - take two Resolve issues with typed data tests and compression. Uses HeaderValues to parse permessage_deflate option. Includes changes from https://codereview.chromium.org/1208473005/ BUG=https://github.com/dart-lang/sdk/issues/14441 R=sgjesse@google.com Committed: https://github.com/dart-lang/sdk/commit/59bb84127b63a7516a32a32eae6a6839420709db ========== to ========== Web Socket compression - take two Resolve issues with typed data tests and compression. Uses HeaderValues to parse permessage_deflate option. Includes changes from https://codereview.chromium.org/1208473005/ BUG=https://github.com/dart-lang/sdk/issues/14441 R=sgjesse@google.com Committed: https://github.com/dart-lang/sdk/commit/59bb84127b63a7516a32a32eae6a6839420709db ==========
Message was sent while issue was closed.
Description was changed from ========== Web Socket compression - take two Resolve issues with typed data tests and compression. Uses HeaderValues to parse permessage_deflate option. Includes changes from https://codereview.chromium.org/1208473005/ BUG=https://github.com/dart-lang/sdk/issues/14441 R=sgjesse@google.com Committed: https://github.com/dart-lang/sdk/commit/59bb84127b63a7516a32a32eae6a6839420709db ========== to ========== Web Socket compression - take two Resolve issues with typed data tests and compression. Uses HeaderValues to parse permessage_deflate option. Includes changes from https://codereview.chromium.org/1208473005/ BUG=https://github.com/dart-lang/sdk/issues/14441 R=sgjesse@google.com Committed: https://github.com/dart-lang/sdk/commit/59bb84127b63a7516a32a32eae6a6839420709db Reverted: https://github.com/dart-lang/sdk/commit/d1e01e205a615257ba9391cb531ad8b053af1fed Additional fixes for failing tests. ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) manually as 95bb2159d9fb7f44d3bbc99932a57173e1b464aa (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
