From bb560bb233a3ef4714dc86a0f7882f3d4913e523 Mon Sep 17 00:00:00 2001 From: Ben Konyi Date: Fri, 25 Jan 2019 19:23:56 +0000 Subject: [PATCH] [ VM / dart:io ] Adding to a closed IOSink now throws a StateError Based off of these original changes: https://codereview.chromium.org/2857393003 This is a breaking change and will require an annoucement stating such before landing. Fixes #29554. Change-Id: Ibb56fd49648edc6b9fd567240a3bebb05a14234d Reviewed-on: https://dart-review.googlesource.com/c/90120 Commit-Queue: Ben Konyi Reviewed-by: Siva Annamalai --- CHANGELOG.md | 3 +++ sdk/lib/_http/http_impl.dart | 12 ++--------- sdk/lib/async/stream_transformers.dart | 12 ++--------- sdk/lib/io/io_sink.dart | 20 ++----------------- tests/co19_2/co19_2-kernel.status | 1 - .../io/client_socket_exception_test.dart | 2 +- tests/standalone_2/io/file_test.dart | 4 +++- tests/standalone_2/io/http_10_test.dart | 4 +++- .../io/http_content_length_test.dart | 12 ++++++++--- .../io/socket_upgrade_to_secure_test.dart | 16 +++++++++++---- 10 files changed, 37 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 147810fcf83..2194910ff5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,9 @@ #### `dart:io` * Added ability to get and set low level socket options. +* **Breaking Change:** Adding to a closed `IOSink` now throws a `StateError`. + +[29554]: https://github.com/dart-lang/sdk/issues/29554 ### Dart VM diff --git a/sdk/lib/_http/http_impl.dart b/sdk/lib/_http/http_impl.dart index ecb18e0da4a..f0e365dc47a 100644 --- a/sdk/lib/_http/http_impl.dart +++ b/sdk/lib/_http/http_impl.dart @@ -531,24 +531,16 @@ class _StreamSinkImpl implements StreamSink { _StreamSinkImpl(this._target); - void _reportClosedSink() { - stderr.writeln("StreamSink is closed and adding to it is an error."); - stderr.writeln(" See http://dartbug.com/29554."); - stderr.writeln(StackTrace.current); - } - void add(T data) { if (_isClosed) { - _reportClosedSink(); - return; + throw StateError("StreamSink is closed"); } _controller.add(data); } void addError(error, [StackTrace stackTrace]) { if (_isClosed) { - _reportClosedSink(); - return; + throw StateError("StreamSink is closed"); } _controller.addError(error, stackTrace); } diff --git a/sdk/lib/async/stream_transformers.dart b/sdk/lib/async/stream_transformers.dart index 9a4b1dcdb46..b656dea3dc7 100644 --- a/sdk/lib/async/stream_transformers.dart +++ b/sdk/lib/async/stream_transformers.dart @@ -220,17 +220,9 @@ class _HandlerEventSink implements EventSink { bool get _isClosed => _sink == null; - _reportClosedSink() { - // TODO(29554): throw a StateError, and don't just report the problem. - Zone.root - ..print("Sink is closed and adding to it is an error.") - ..print(" See http://dartbug.com/29554.") - ..print(StackTrace.current.toString()); - } - void add(S data) { if (_isClosed) { - _reportClosedSink(); + throw StateError("Sink is closed"); } if (_handleData != null) { _handleData(data, _sink); @@ -241,7 +233,7 @@ class _HandlerEventSink implements EventSink { void addError(Object error, [StackTrace stackTrace]) { if (_isClosed) { - _reportClosedSink(); + throw StateError("Sink is closed"); } if (_handleError != null) { _handleError(error, stackTrace, _sink); diff --git a/sdk/lib/io/io_sink.dart b/sdk/lib/io/io_sink.dart index e8e4a086f8b..5481265742f 100644 --- a/sdk/lib/io/io_sink.dart +++ b/sdk/lib/io/io_sink.dart @@ -149,32 +149,16 @@ class _StreamSinkImpl implements StreamSink { _StreamSinkImpl(this._target); - void _reportClosedSink() { - // TODO(29554): this is very brittle and depends on the layout of the - // stderr class. - if (this == stderr._sink) { - // We can't report on stderr anymore (as we would otherwise - // have an infinite recursion. - throw new StateError("Stderr is closed."); - } - // TODO(29554): throw a StateError, and don't just report the problem. - stderr.writeln("StreamSink is closed and adding to it is an error."); - stderr.writeln(" See http://dartbug.com/29554."); - stderr.writeln(StackTrace.current); - } - void add(T data) { if (_isClosed) { - _reportClosedSink(); - return; + throw StateError("StreamSink is closed"); } _controller.add(data); } void addError(error, [StackTrace stackTrace]) { if (_isClosed) { - _reportClosedSink(); - return; + throw StateError("StreamSink is closed"); } _controller.addError(error, stackTrace); } diff --git a/tests/co19_2/co19_2-kernel.status b/tests/co19_2/co19_2-kernel.status index d011d229bee..76534bd9319 100644 --- a/tests/co19_2/co19_2-kernel.status +++ b/tests/co19_2/co19_2-kernel.status @@ -1367,7 +1367,6 @@ LibTest/io/HttpClientBasicCredentials/HttpClientBasicCredentials_A01_t01: Runtim LibTest/io/HttpClientDigestCredentials/HttpClientDigestCredentials_A01_t01: RuntimeError LibTest/io/HttpClientRequest/addStream_A02_t02: RuntimeError LibTest/io/HttpClientRequest/add_A03_t01: RuntimeError -LibTest/io/HttpClientRequest/close_A02_t01: RuntimeError LibTest/io/HttpClientRequest/done_A02_t01: RuntimeError, Timeout, Pass LibTest/io/HttpClientResponse/certificate_A01_t01: RuntimeError LibTest/io/HttpClientResponse/isRedirect_A01_t02: RuntimeError diff --git a/tests/standalone_2/io/client_socket_exception_test.dart b/tests/standalone_2/io/client_socket_exception_test.dart index 647804c10e2..b6c53a284aa 100644 --- a/tests/standalone_2/io/client_socket_exception_test.dart +++ b/tests/standalone_2/io/client_socket_exception_test.dart @@ -47,7 +47,7 @@ void clientSocketExceptionTest() { } catch (ex) { wrongExceptionCaught = true; } - Expect.isFalse(exceptionCaught); + Expect.isTrue(exceptionCaught); Expect.isFalse(wrongExceptionCaught); // From here exceptions are expected. diff --git a/tests/standalone_2/io/file_test.dart b/tests/standalone_2/io/file_test.dart index 3715504fcb8..74f57f76ab8 100644 --- a/tests/standalone_2/io/file_test.dart +++ b/tests/standalone_2/io/file_test.dart @@ -988,7 +988,9 @@ class FileTest { file.createSync(); var output = file.openWrite(); output.close(); - output.add(buffer); // Ignored. + Expect.throws(() { + output.add(buffer); + }); output.done.then((_) { file.deleteSync(); asyncTestDone("testCloseExceptionStream"); diff --git a/tests/standalone_2/io/http_10_test.dart b/tests/standalone_2/io/http_10_test.dart index 9a02f73a5e1..92ecc6fdf97 100644 --- a/tests/standalone_2/io/http_10_test.dart +++ b/tests/standalone_2/io/http_10_test.dart @@ -29,7 +29,9 @@ void testHttp10NoKeepAlive() { response.write("Z"); response.write("Z"); response.close(); - response.write("x"); + Expect.throws(() { + response.write("x"); + }, (e) => e is StateError); }, onError: (e, trace) { String msg = "Unexpected error $e"; if (trace != null) msg += "\nStackTrace: $trace"; diff --git a/tests/standalone_2/io/http_content_length_test.dart b/tests/standalone_2/io/http_content_length_test.dart index 6f676640d2b..15d65204e6a 100644 --- a/tests/standalone_2/io/http_content_length_test.dart +++ b/tests/standalone_2/io/http_content_length_test.dart @@ -37,7 +37,9 @@ void testNoBody(int totalConnections, bool explicitContentLength) { // After an explicit close, write becomes a state error // because we have said we will not add more. response.close(); - response.write("x"); + Expect.throws(() { + response.write("x"); + }, (e) => e is StateError); }, onError: (e, trace) { String msg = "Unexpected server error $e"; if (trace != null) msg += "\nStackTrace: $trace"; @@ -89,7 +91,9 @@ void testBody(int totalConnections, bool useHeader) { } }); response.close(); - response.write("x"); + Expect.throws(() { + response.write("x"); + }, (e) => e is StateError); }); }, onError: (e, trace) { String msg = "Unexpected error $e"; @@ -149,7 +153,9 @@ void testBodyChunked(int totalConnections, bool useHeader) { response.write("x"); response.write("x"); response.close(); - response.write("x"); + Expect.throws(() { + response.write("x"); + }, (e) => e is StateError); }); }, onError: (e, trace) { String msg = "Unexpected error $e"; diff --git a/tests/standalone_2/io/socket_upgrade_to_secure_test.dart b/tests/standalone_2/io/socket_upgrade_to_secure_test.dart index 13957ca7e3c..9209f34085b 100644 --- a/tests/standalone_2/io/socket_upgrade_to_secure_test.dart +++ b/tests/standalone_2/io/socket_upgrade_to_secure_test.dart @@ -164,7 +164,9 @@ void test(bool hostnameInConnect, bool handshakeBeforeSecure, SecureSocket.secure(socket, host: HOST, context: clientContext); } return future.then((SecureSocket secureSocket) { - socket.add([0]); + Expect.throws(() { + socket.add([0]); + }); return secureSocket; }); }); @@ -179,7 +181,9 @@ void test(bool hostnameInConnect, bool handshakeBeforeSecure, SecureSocket.secure(socket, host: HOST, context: clientContext); } return future.then((secureSocket) { - socket.add([0]); + Expect.throws(() { + socket.add([0]); + }); return secureSocket; }); }); @@ -191,7 +195,9 @@ void test(bool hostnameInConnect, bool handshakeBeforeSecure, server.listen((client) { if (!handshakeBeforeSecure) { SecureSocket.secureServer(client, serverContext).then((secureClient) { - client.add([0]); + Expect.throws(() { + client.add([0]); + }); runServer(secureClient).then((_) => server.close()); }); } else { @@ -199,7 +205,9 @@ void test(bool hostnameInConnect, bool handshakeBeforeSecure, SecureSocket .secureServer(client, serverContext, bufferedData: carryOverData) .then((secureClient) { - client.add([0]); + Expect.throws(() { + client.add([0]); + }); runServer(secureClient).then((_) => server.close()); }); });