Fix secure socket tests to close sockets after writing. Fix handling of close events in TlsSocket …

Fix secure socket tests to close sockets after writing.  Fix handling of close events in TlsSocket class.


BUG=

Committed: https://code.google.com/p/dart/source/detail?r=15265

[Mads Ager (google), 2012-11-21 16:06:31]

https://codereview.chromium.org/11417116/diff/2001/sdk/lib/io/tls_socket.dart
File sdk/lib/io/tls_socket.dart (right):

https://codereview.chromium.org/11417116/diff/2001/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:233: _socketWriteHandler = null;
I don't understand this change. Why would you want to get rid of the socket
write handler here?

https://codereview.chromium.org/11417116/diff/2001/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:397: // TODO(whesse): Fix this as part of fixing read
and readList.
What does _readEncryptedData() do in that case? Is it harmless?

https://codereview.chromium.org/11417116/diff/2001/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:420: bool _filterReadEmpty = null;  // There is no
buffered data to read.
Please initialize to booleans. It would make sense that they are true on
initialization as there is nothing in the filter at that point.

[Bill Hesse, 2012-11-21 16:25:25]

https://codereview.chromium.org/11417116/diff/2001/sdk/lib/io/tls_socket.dart
File sdk/lib/io/tls_socket.dart (right):

https://codereview.chromium.org/11417116/diff/2001/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:233: _socketWriteHandler = null;
On 2012/11/21 16:06:31, Mads Ager wrote:
> I don't understand this change. Why would you want to get rid of the socket
> write handler here?

The onWrite handler is a one-shot handler, that is reset to null after being
called.  This is in the documentation of the Socket class.  It was a mistake
that we were potentially calling it multiple times here. 

There was an error here, in that it should be possible to reset the write
handler from the write handler.  Fixed.
There is another potential bug:
 I have now added a check here that it is possible to write to the plaintext
write buffer here.

https://codereview.chromium.org/11417116/diff/2001/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:397: // TODO(whesse): Fix this as part of fixing read
and readList.
On 2012/11/21 16:06:31, Mads Ager wrote:
> What does _readEncryptedData() do in that case? Is it harmless?

Yes, _readEncryptedData is harmless - it advances data through the buffers of
the filter pipeline.  It checks that the socket has not been closed for reading
before attempting to read data from it.

[Bill Hesse, 2012-11-21 17:30:40]

I also fixed the issue with incomplete writes, by adding a helper function
WriteAndClose to both tls_socket_test.dart and tls_server_test.dart.

[Mads Ager (google), 2012-11-22 10:04:16]

LGTM

https://codereview.chromium.org/11417116/diff/2001/sdk/lib/io/tls_socket.dart
File sdk/lib/io/tls_socket.dart (right):

https://codereview.chromium.org/11417116/diff/2001/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:233: _socketWriteHandler = null;
On 2012/11/21 16:25:25, Bill Hesse wrote:
> On 2012/11/21 16:06:31, Mads Ager wrote:
> > I don't understand this change. Why would you want to get rid of the socket
> > write handler here?
> 
> The onWrite handler is a one-shot handler, that is reset to null after being
> called.  This is in the documentation of the Socket class.  It was a mistake
> that we were potentially calling it multiple times here. 
> 
> There was an error here, in that it should be possible to reset the write
> handler from the write handler.  Fixed.
> There is another potential bug:
>  I have now added a check here that it is possible to write to the plaintext
> write buffer here.

Thanks, that's right. I keep forgetting that the writeHandler is one-shot. With
the new event-handling system I think this should just be a stream that you need
to subscribe to and unsubscribe from like any other stream. But this is right
for this change. :)

https://codereview.chromium.org/11417116/diff/9001/sdk/lib/io/tls_socket.dart
File sdk/lib/io/tls_socket.dart (right):

https://codereview.chromium.org/11417116/diff/9001/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:231: if (_socketWriteHandler != null &&
Combine this with the else if?

else if (status == CONNECTED &&
         _socketWriteHandler != null &&
         _tlsFilter.... > 0) {
  doStuff();
}

https://codereview.chromium.org/11417116/diff/9001/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:235: _socketWriteHandler = null;  // Reset the
one-shot handler.
nit: I prefer the comment on the line before.