Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1208473005:
WebSocket Compression (Closed)
|
Created:
5 years, 6 months ago by kaendfinger Modified:
5 years, 1 month ago CC:
reviews_dartlang.org Base URL:
https://github.com/dart-lang/sdk.git Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionWebSocket Compression
BUG: https://code.google.com/p/dart/issues/detail?id=14441
Patch Set 1 #
Total comments: 16
Patch Set 2 : Take Suggestions #
Total comments: 5
Patch Set 3 : Take Suggestions #
Total comments: 7
Patch Set 4 : Provide all changes #Patch Set 5 : Fix a lot of issues #Patch Set 6 : Bug Fixes and Improvements #Patch Set 7 : Format Code #Patch Set 8 : Add Test #Patch Set 9 : Remove Debugging Line #
Total comments: 18
Messages
Total messages: 13 (3 generated)
kaendfinger@gmail.com changed reviewers: + sgjesse@google.com
Thanks for working on this. Here is the first round of comments. Please add some tests. We do have the test "web_socket_protocol_processor_test.dart" where we simulate the dart.io library and bring in part files from dart.io to test internals - maybe you can use that. https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:44: static const CompressionOptions OFF = const CompressionOptions(enabled: false); Long line, see https://www.dartlang.org/articles/style-guide/#avoid-lines-longer-than-80-cha.... https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:71: const CompressionOptions({this.clientNoContextTakeover: false, Please reformat the arguments, see https://www.dartlang.org/articles/style-guide/#do-indent-continued-lines-with.... https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:89: var myMaxWindowBits = clientMaxWindowBits == null ? 15 : clientMaxWindowBits; Long line. https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket.dart#n... sdk/lib/io/websocket.dart:94: if (clientNoContextTakeover && (requested != null Please look at https://www.dartlang.org/articles/style-guide/#do-place-binary-operators-on-t... regarding formatting of binary operators. https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket_impl.dart File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:112: _fin = (byte & 0x80) != 0; Please add constants for FIN, RSV1, RSV2 and RSV3 bit-patterns. https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:123: _opcode = (byte & 0xF); Please add a constant for the opcode mask. https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:423: Extract server side negotiation into a separate method. https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:439: .then((socket) => new _WebSocketImpl._fromSocket( Compression not supported for server (no additional optional argument passed)? https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:506: _WebSocketPerMessageDeflate({this.clientMaxWindowBits, No need to use optional named argument for internal private classes. https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:509: if (clientMaxWindowBits == null) { Make the caller always pass a non null value - it must know from the negoation. https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:510: clientMaxWindowBits = 15; Create a constant for this (maybe in CompressionOptions). https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:536: builder.add(const [0x00, 0x00, 0xff, 0xff]); For the noContextTakeover case set decoder to null, or figure out if there is some way to reset it to not consume space. https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:543: c = c.sublist(0, c.length - 4); Ditto. https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:921: ..set("Sec-WebSocket-Extensions", "permessage-deflate"); Isn't this already handled by adding Sec-WebSocket-Extensions below? https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:963: Please extract this into a separate method called something like negotiateClientCompression. https://codereview.chromium.org/1208473005/diff/1/sdk/lib/io/websocket_impl.d... sdk/lib/io/websocket_impl.dart:989: o = o.substring("client_max_window_bits=".length); client -> ${type} here as well? Use var parameter = "${type}_max_window_bits=" to only have this once.
https://codereview.chromium.org/1208473005/diff/20001/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/1208473005/diff/20001/sdk/lib/io/websocket.da... sdk/lib/io/websocket.dart:87: var header = "permessage-deflate"; Please add constants for all these strings. https://codereview.chromium.org/1208473005/diff/20001/sdk/lib/io/websocket.da... sdk/lib/io/websocket.dart:94: clientMaxWindowBits == null ? 15 : clientMaxWindowBits; Please use a constant for 15. https://codereview.chromium.org/1208473005/diff/20001/sdk/lib/io/websocket_im... File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/1208473005/diff/20001/sdk/lib/io/websocket_im... sdk/lib/io/websocket_impl.dart:472: Iterable<List<String>> extensions = extensionHeader.split(",").map((it) => it.split("; ")); Long line (more below). https://codereview.chromium.org/1208473005/diff/20001/sdk/lib/io/websocket_im... sdk/lib/io/websocket_impl.dart:476: response.headers.add("Sec-WebSocket-Extensions", compression._createHeader(opts)); Wouldnt it be better here to take the extenstion (the "opts") and parse it into a CompressionOptions object (e.g. static method CompressionOptions.parse), and then have a methot ti "intersect" two CompressionOptions objects? It could be a method on a CompressionOptions that takes an other CompressionOptions and returns a new CompressionOptions whit the effective optins. https://codereview.chromium.org/1208473005/diff/20001/sdk/lib/io/websocket_im... sdk/lib/io/websocket_impl.dart:1008: var opts = extensions.firstWhere((x) => x[0] == "permessage-deflate"); As mentioned above parsing "opts" int a CompressionOptions object could make this simpler.
kustermann@google.com changed reviewers: + kustermann@google.com
Thanks for the update. Soeren is on vacation ATM. I tried to take a look, but it looks like this is building upon other changes which haven't been committed to the dart repository. Where is the the base code this CL changes? Also: Do you have some tests for the websocket compression code? [I'm not sure I can review this, since it builds upon other code I haven't seen. If you don't mind, you could wait with this until Soeren is back?] https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket.da... sdk/lib/io/websocket.dart:94: clientMaxWindowBits == null ? _WebSocketImpl.DEFAULT_WINDOW_BITS : clientMaxWindowBits; long line https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket.da... sdk/lib/io/websocket.dart:101: requested.contains("client_no_context_takeover"))) { Indentation seems wrong. Would be nice if you could align the conditions on the left. https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket.da... sdk/lib/io/websocket.dart:107: requested.contains("server_no_context_takeover"))) { ditto https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket.da... sdk/lib/io/websocket.dart:112: var mwb = serverMaxWindowBits == null ? _WebSocketImpl.DEFAULT_WINDOW_BITS : serverMaxWindowBits; long line https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket_im... File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket_im... sdk/lib/io/websocket_impl.dart:465: CompressionOptions compression) { Consider using dart_style for automatic indentation. https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket_im... sdk/lib/io/websocket_impl.dart:477: var opts = extensions.firstWhere((x) => x[0] == _WebSocketImpl.PER_MESSAGE_DEFLATE); Seems redundant to do two traversals of [extensions]. Maybe just do one 'extensions.firstWhere(...)' and make `null` the default. https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket_im... sdk/lib/io/websocket_impl.dart:477: var opts = extensions.firstWhere((x) => x[0] == _WebSocketImpl.PER_MESSAGE_DEFLATE); Normally we don't abbreviate, meaning opts -> options.
On 2015/07/20 09:34:12, kustermann wrote: > Thanks for the update. Soeren is on vacation ATM. I tried to take a look, but it > looks like this is building upon other changes which haven't been committed to > the dart repository. Where is the the base code this CL changes? > > Also: Do you have some tests for the websocket compression code? > > [I'm not sure I can review this, since it builds upon other code I haven't seen. > If you don't mind, you could wait with this until Soeren is back?] > > https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket.dart > File sdk/lib/io/websocket.dart (right): > > https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket.da... > sdk/lib/io/websocket.dart:94: clientMaxWindowBits == null ? > _WebSocketImpl.DEFAULT_WINDOW_BITS : clientMaxWindowBits; > long line > > https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket.da... > sdk/lib/io/websocket.dart:101: > requested.contains("client_no_context_takeover"))) { > Indentation seems wrong. Would be nice if you could align the conditions on the > left. > > https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket.da... > sdk/lib/io/websocket.dart:107: > requested.contains("server_no_context_takeover"))) { > ditto > > https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket.da... > sdk/lib/io/websocket.dart:112: var mwb = serverMaxWindowBits == null ? > _WebSocketImpl.DEFAULT_WINDOW_BITS : serverMaxWindowBits; > long line > > https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket_im... > File sdk/lib/io/websocket_impl.dart (right): > > https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket_im... > sdk/lib/io/websocket_impl.dart:465: CompressionOptions compression) { > Consider using dart_style for automatic indentation. > > https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket_im... > sdk/lib/io/websocket_impl.dart:477: var opts = extensions.firstWhere((x) => x[0] > == _WebSocketImpl.PER_MESSAGE_DEFLATE); > Seems redundant to do two traversals of [extensions]. Maybe just do one > 'extensions.firstWhere(...)' and make `null` the default. > > https://codereview.chromium.org/1208473005/diff/40001/sdk/lib/io/websocket_im... > sdk/lib/io/websocket_impl.dart:477: var opts = extensions.firstWhere((x) => x[0] > == _WebSocketImpl.PER_MESSAGE_DEFLATE); > Normally we don't abbreviate, meaning opts -> options. I'm unsure how I would get the rest of the changes on the CL. Everything is on a GitHub branch though. https://github.com/kaendfinger/sdk/tree/websocket-compress I can take the suggestions here, and then write some tests. If you could provide some insight into how I could get the rest of the changes in the CL, that would be awesome too. Thanks!
I have written a test for WebSocket compression now. We are actually using it in our benchmarks for our DSA SDKs, so I can confirm it is working in our use case.
Patchset #9 (id:160001) has been deleted
In general this is looking very good. I have a number of comments in relation to the header parsing, but when that is fixed this is ready to land. One thing that would be really nice to test is the different header parameter formatting, (white space, quoted/not quoted) e.g. server_no_context_takeover; server_max_window_bits = "10" That would require connecting a socket directly and building the headers like _upgrade in websocket_impl.dart does in a test. https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket.dart File sdk/lib/io/websocket.dart (right): https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket.d... sdk/lib/io/websocket.dart:80: * Create a Compression Header Please add more documentation here on what requested is, and what is returned - it doesn't just create a compression header. As far as I can see if requested is not passed this generates a client request, and if requested is passed it generates the server response. Maybe add methods String _createClientRequestHeader() List _createServerResponseHeader(requested) that forward to _createHeader. https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket.d... sdk/lib/io/websocket.dart:82: List _createHeader([List<String> requested]) { Pass HeaderValue here and use its parameters (see comment in next file). https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket.d... sdk/lib/io/websocket.dart:93: requested.contains("client_no_context_takeover"))) { 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"; ... https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket.d... sdk/lib/io/websocket.dart:104: requested.any((x) => x.startsWith("server_max_window_bits="))) { This check is fragile, there can be WS on both sides of the = sign. See comments in next file on parsing parameters. https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket.d... sdk/lib/io/websocket.dart:109: ? 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/1208473005/diff/180001/sdk/lib/io/websocket.d... sdk/lib/io/websocket.dart:111: : serverMaxWindowBits; You also need to check for the allowed range 8-15. https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket_i... File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket_i... sdk/lib/io/websocket_impl.dart:115: var rsv = (byte & 0x70) >> 4; I don't think this temp is required - see below. https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket_i... sdk/lib/io/websocket_impl.dart:117: if (rsv != 0 && rsv != 4) { Please use if ((byte & (RSV1 | RSV2)) != 0) to test against the defined mask bits https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket_i... sdk/lib/io/websocket_impl.dart:118: // The RSV1, RSV2 bits RSV3 must be all zero. Please remove RSV3 from this comment. https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket_i... sdk/lib/io/websocket_impl.dart:122: if (rsv == 4) { Please use if ((byte & RSV3) != 0) { to test against the defined mask bit https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket_i... sdk/lib/io/websocket_impl.dart:127: _opcode = (byte & 0x0F); Please use the new constant OPCODE here. https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket_i... sdk/lib/io/websocket_impl.dart:464: extensionHeader.split(",").map((it) => it.split("; ")); Both the splitting using "," and "; " seems fragile. The "," could be inside a quoted parameter value, and separation with just ";" is also OK. We do have the problem with dart:io parsing into HttpHeader that the "," splitting is not performed by default. For now please add a TODO that the "," splitting should do proper parsing of multiple header values. For the parsing of parameters I think you can just use the HeaderValue parameter parsing: var value = HeaderValue.parse(s); the value.parameters has the parameter map. There is one slight issue with this, and that is that the current HeaderValue parser requires the format 'x=1' - it does not support just 'x'. That should be simple to add to http_headers.dart (around line 747) to check for either '=' or ';' and give a value of null when there is no value. https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket_i... sdk/lib/io/websocket_impl.dart:529: {this.clientMaxWindowBits, nit: Please indent like this (one space more for the lines under the {-line). {this.clientMaxWindowBits, this.serverMaxWindowBits, this.serverNoContextTakeover: false, ... There are more places like this. https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket_i... sdk/lib/io/websocket_impl.dart:719: header[index++] = hoc; Just set this byte like this (no need to the rsv and hoc temps): header[index++] = FIN | compressed ? RSV3 : 0 | opcode https://codereview.chromium.org/1208473005/diff/180001/sdk/lib/io/websocket_i... sdk/lib/io/websocket_impl.dart:1037: extensionHeader.split(", ").map((it) => it.split("; ")); Please see above (comment on _negoatiateCompression) on the parsing of the header. https://codereview.chromium.org/1208473005/diff/180001/tests/standalone/io/we... File tests/standalone/io/web_socket_test.dart (right): https://codereview.chromium.org/1208473005/diff/180001/tests/standalone/io/we... tests/standalone/io/web_socket_test.dart:552: Please create a new test file web_socket_compression_test.dart (just copy createServer and other helpers needed). https://codereview.chromium.org/1208473005/diff/180001/tests/standalone/io/we... tests/standalone/io/web_socket_test.dart:553: void testCompressionSupport(bool enabled, bool allowContextTakeover) { Please add tests involving the max_window_bits as well. https://codereview.chromium.org/1208473005/diff/180001/tests/standalone/io/we... tests/standalone/io/web_socket_test.dart:564: WebSocketTransformer.upgrade(request, compression: options).then((webSocket) { nit: Long line. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
