Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(999)

Issue 1133673006: Make sure to process WebSocket frames when closing a WebSocket (Closed)

Created:
5 years, 7 months ago by Søren Gjesse
Modified:
5 years, 6 months ago
CC:
reviews_dartlang.org
Target Ref:
refs/remotes/git-svn
Visibility:
Public.

Description

Make sure to process WebSocket frames when closing a WebSocket On a WebSocket the actual listening on the stream is causing the processing of inbound frames to start. If the connection is closed without ever being listened on, then a close frame from the other end will not be processed. In this case the timeout of waiting for a close from the other end will always be hit. If you are _only_ using the WebSocket connection in one direction it seems a bit artificial to have to listen on the stream to even process the close frame. This change will make sure the stream is listened to when closing a WebSocket. This is a breaking change. Before you could close and then listen. With this change that sequence will throw on the listen. This is reflected in the changes that was needed for the tests. R=kustermann@google.com, lrn@google.com BUG= Committed: https://code.google.com/p/dart/source/detail?r=45679

Patch Set 1 #

Total comments: 6

Patch Set 2 : Addressed review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+56 lines, -13 lines) Patch
M sdk/lib/io/websocket_impl.dart View 1 1 chunk +20 lines, -10 lines 0 comments Download
M tests/standalone/io/web_socket_pipe_test.dart View 2 chunks +2 lines, -1 line 0 comments Download
M tests/standalone/io/web_socket_test.dart View 1 4 chunks +34 lines, -2 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
Søren Gjesse
5 years, 7 months ago (2015-05-08 13:03:40 UTC) #1
kustermann
lgtm with comments https://codereview.chromium.org/1133673006/diff/1/sdk/lib/io/websocket_impl.dart File sdk/lib/io/websocket_impl.dart (right): https://codereview.chromium.org/1133673006/diff/1/sdk/lib/io/websocket_impl.dart#newcode969 sdk/lib/io/websocket_impl.dart:969: _controller.stream.drain(); This can cause an un-catchable ...
5 years, 7 months ago (2015-05-08 13:21:20 UTC) #2
Søren Gjesse
Committed patchset #2 (id:20001) manually as 45679 (presubmit successful).
5 years, 7 months ago (2015-05-11 09:33:02 UTC) #3
Søren Gjesse
5 years, 6 months ago (2015-06-15 07:51:32 UTC) #4
Message was sent while issue was closed.
These are old reply comments I have forgot to send.

https://codereview.chromium.org/1133673006/diff/1/sdk/lib/io/websocket_impl.dart
File sdk/lib/io/websocket_impl.dart (right):

https://codereview.chromium.org/1133673006/diff/1/sdk/lib/io/websocket_impl.d...
sdk/lib/io/websocket_impl.dart:969: _controller.stream.drain();
On 2015/05/08 13:21:20, kustermann wrote:
> This can cause an un-catchable exception. You need to do put a catchError on
it.

Thanks. Added catchError which ignored the error.

https://codereview.chromium.org/1133673006/diff/1/tests/standalone/io/web_soc...
File tests/standalone/io/web_socket_test.dart (right):

https://codereview.chromium.org/1133673006/diff/1/tests/standalone/io/web_soc...
tests/standalone/io/web_socket_test.dart:185: });
On 2015/05/08 13:21:20, kustermann wrote:
> indentation

Done.

https://codereview.chromium.org/1133673006/diff/1/tests/standalone/io/web_soc...
tests/standalone/io/web_socket_test.dart:201: });
On 2015/05/08 13:21:20, kustermann wrote:
> indentation

Done.

Powered by Google App Engine
This is Rietveld 408576698