Issue 3346 in pharo: Socket bug fixes

Status: FixedWaitingToBePharoed
Owner: stephane.ducasse
Labels: Milestone-1.3

New issue 3346 by stephane.ducasse: Socket bug fixes
http://code.google.com/p/pharo/issues/detail?id=3346

Andreas Raab uploaded a new version of Network to project The Trunk:
http://source.squeak.org/trunk/Network-ar.98.mcz

==================== Summary ====================

Name: Network-ar.98
Author: ar
Time: 23 November 2010, 12:12:58.498 am
UUID: 5f0637f3-9b53-ab42-a6fa-55077db31f63
Ancestors: Network-ul.97

Fixes for SocketStream:
1) SocketStream>>receiveData: and SocketStream>>upToEnd: had incorrect  
conditions used to determine when to stop (#isConnected is wrong since  
there can be data pending on an unconnected socket which is why using  
#atEnd is the correct test).
2) The former was done to deal with non-signaling SocketStreams in cases  
where ConnectionClose was used to deal with end conditions (#upToEnd,  
#next:into:startingAt:, #readInto:startingAt:count:). This was fixed by  
making these places (temporarily) signaling so that the exception can be  
caught and handled properly.
3) SocketStream>>isDataAvailable should not attempt to shortcut  
prematurely; using 'socket dataAvailable' is bad for subclasses such as  
SecureSocketStream and unnecessary to boot (and has no performance impact).

=============== Diff against Network-ul.97 ===============

Item was added:
+ ----- Method: SocketStream>>beSignalingWhile: (in category 'private')  
-----
+ beSignalingWhile: aBlock
+       "Temporarily turn a non-signaling SocketStream into a signaling one.
+       Required for some of operations that will catch ConnectionClosed in
+       order to find out that an operation completed"
+
+       | signaling |
+       signaling := shouldSignal.
+       shouldSignal := true.
+       ^aBlock ensure:[shouldSignal := signaling]
+ !

Item was changed:
  ----- Method: SocketStream>>isDataAvailable (in category 'testing') -----
  isDataAvailable
+       "Answer if more data can be read. It the inbuffer is empty, we read  
more data.
-       "Answer if more data can be read. It the inbuffer is empty, we  
check the socket for data. If it claims to have data available to read, we  
try to read some once and recursively call this method again. If something  
really was available it is now in the inBuffer. This is because there has  
been spurious dataAvailable when there really is no data to get.

+       Note: It is important not to rely on 'socket dataAvailable' here  
since this will
+       not work for subclasses such as SecureSocketStream (which can  
contain
+       undecrypted contents that has been read from the socket)."
-       Note: Some subclasses (such as SecureSocketStream) rely on the  
behavior here since even though data may be available in the underlying  
socket, it may not result in any output (yet)."

        self isInBufferEmpty ifFalse: [^true].
+       ^self receiveAvailableData < inNextToWrite
+ !
-       ^socket dataAvailable
-               ifFalse: [false]
-               ifTrue: [self receiveAvailableData; isDataAvailable]!

Item was changed:
  ----- Method: SocketStream>>next:into:startingAt: (in category 'stream  
in') -----
  next: anInteger into: aCollection startingAt: startIndex
        "Read n objects into the given collection.
        Return aCollection or a partial copy if less than
        n elements have been read."

        "Implementation note: This method DOES signal timeout if not
        enough elements are received. It does NOT signal
        ConnectionClosed as closing the connection is the only way by
        which partial data can be read."

        | start amount |

+       [self beSignalingWhile:[self receiveData: anInteger]]
+               on: ConnectionClosed do:[:ex| ex return].
-       [self receiveData: anInteger] on: ConnectionClosed do:[:ex| ex  
return].

        "Inlined version of nextInBuffer: to avoid copying the contents"
        amount := anInteger min: (inNextToWrite - lastRead - 1).
        start := lastRead + 1.
        lastRead := lastRead + amount.
        aCollection
                replaceFrom: startIndex
                to: startIndex + amount-1
                with: inBuffer
                startingAt: start.
        ^amount < anInteger
                ifTrue:[aCollection copyFrom: 1 to:  startIndex + amount-1]
                ifFalse:[aCollection]!

Item was changed:
  ----- Method: SocketStream>>readInto:startingAt:count: (in  
category 'stream in') -----
  readInto: aCollection startingAt: startIndex count: anInteger
        "Read n objects into the given collection starting at startIndex.
        Return number of elements that have been read."

        "Implementation note: This method DOES signal timeout if not
        enough elements are received. It does NOT signal
        ConnectionClosed as closing the connection is the only way by
        which partial data can be read."

        | start amount |

+       [self beSignalingWhile:[self receiveData: anInteger]]
+               on: ConnectionClosed do:[:ex| ex return].
-       [self receiveData: anInteger] on: ConnectionClosed do:[:ex| ex  
return].

        "Inlined version of nextInBuffer: to avoid copying the contents"
        amount := anInteger min: (inNextToWrite - lastRead - 1).
        start := lastRead + 1.
        lastRead := lastRead + amount.
        aCollection
                replaceFrom: startIndex
                to: startIndex + amount-1
                with: inBuffer
                startingAt: start.
        ^amount!

Item was changed:
  ----- Method: SocketStream>>receiveData: (in category 'control') -----
  receiveData: nBytes
        "Keep reading the socket until we have nBytes
        in the inBuffer or we reach the end. This method
        does not return data, but can be used to make sure
        data has been read into the buffer from the Socket
        before actually reading it from the FastSocketStream.
        Mainly used internally. We could also adjust the buffer
        to the expected amount of data and avoiding several
        incremental grow operations.

        NOTE: This method doesn't honor timeouts if shouldSignal
        is false!! And frankly, I am not sure how to handle that
        case or if I care - I think we should always signal."

+       [self atEnd not and: [nBytes > self inBufferSize]]
-       [self isConnected and: [nBytes > self inBufferSize]]
                whileTrue: [self receiveData]!

Item was changed:
  ----- Method: SocketStream>>upToEnd (in category 'stream in') -----
  upToEnd
        "Answer all data coming in on the socket until the socket
        is closed by the other end, or we get a timeout.
+       This means this method catches ConnectionClosed by itself."
-       This means this method catches ConnectionClosed by itself.
-
-       NOTE: Does not honour timeouts if shouldSignal is false!!"

+       [[self atEnd] whileFalse: [self beSignalingWhile:[self  
receiveData]]]
-       [[self isConnected] whileTrue: [self receiveData]]
                on: ConnectionClosed
                do: [:ex | "swallow it"].
        ^self nextAllInBuffer!

Updates:
        Labels: -Milestone-1.3

Comment #1 on issue 3346 by [hidden email]: Socket bug fixes
http://code.google.com/p/pharo/issues/detail?id=3346

(No comment was entered for this change.)