[Pharo-project] SocketStream>>#next:
Hi, Related to a hard to find Zinc bug that I just discovered, I require some feedback as to the semantics of SocketStream>>#next: IMHO, the current implementation is semantically wrong. I would expect the semantics to be: try to read count elements and return a new species collection containing them. If less elements are available, return a smaller collection. ( It then becomes effectively a limited #upToEnd). I always implemented #next: by delegating to #next:into:, to #next:into:startingAt: and finally #readInto:startingAt:count: (see for example ZdcAbstractSocketStream, ZnLimitedReadStream or ZnChunkedReadStream). When you look at the implementations of #next:into:, #next:into:startingAt: and #readInto:startingAt:count: on SocketStream, the fact that ConnectionClosed is not signalled is clearly stated. Yet SocketStream>>#next: is different, it does signal ConnectionClosed, which makes it impossible (unless you proceed the exception) to read up to end. And this is what InflateStream>>#getFirstBuffer expects. And which fails when less than 65536 bytes are available. Note that InflateStream reads its subsequent buffers using #next:into:startingAt: (see #moveSourceToFront) so that it properly reads up to end without signalling ConnectionClosed for larger payloads. I can easily program around it, but I think SocketStream>>#next: is wrong and should be changed. Any opinions ? Sven -- Sven Van Caekenberghe http://stfx.eu Smalltalk is the Red Pill
On 21 March 2013 20:48, Sven Van Caekenberghe <sven@stfx.eu> wrote:
Hi,
Related to a hard to find Zinc bug that I just discovered, I require some feedback as to the semantics of SocketStream>>#next:
IMHO, the current implementation is semantically wrong. I would expect the semantics to be: try to read count elements and return a new species collection containing them. If less elements are available, return a smaller collection. ( It then becomes effectively a limited #upToEnd).
I always implemented #next: by delegating to #next:into:, to #next:into:startingAt: and finally #readInto:startingAt:count: (see for example ZdcAbstractSocketStream, ZnLimitedReadStream or ZnChunkedReadStream).
When you look at the implementations of #next:into:, #next:into:startingAt: and #readInto:startingAt:count: on SocketStream, the fact that ConnectionClosed is not signalled is clearly stated. Yet SocketStream>>#next: is different, it does signal ConnectionClosed, which makes it impossible (unless you proceed the exception) to read up to end. And this is what InflateStream>>#getFirstBuffer expects. And which fails when less than 65536 bytes are available. Note that InflateStream reads its subsequent buffers using #next:into:startingAt: (see #moveSourceToFront) so that it properly reads up to end without signalling ConnectionClosed for larger payloads.
I can easily program around it, but I think SocketStream>>#next: is wrong and should be changed.
Any opinions ?
Sven
I agree, the implementation notes of #next: should be: - read the N elements from receiver, answer a collection of read elements. if receiver at end, or will reach end before reading all N elements, answer a collection of elements which were successfully read. .. the caller can always check the collection size, and if it < N, (and he insists on having N) he can either throw an error or handle it more graciously. '' readStream next: 5 answering '' so, it actually consistent with what i stating. A reference implementation (in Stream), however doesn't works like that: next: anInteger "Answer the next anInteger number of objects accessible by the receiver." | aCollection | aCollection := OrderedCollection new. anInteger timesRepeat: [aCollection addLast: self next]. ^aCollection this code will always answer collection of N elements.. the bad side of it, that if stream reaching end, the tail will be filled with 'userful' nils. :) so , based on ReadStream implementation we can assume that caller already expects to receive <=N elements, but not N with partly filled with nils. Because then you cannot distinguish whether this nil goes from stream data or from stream itself, e.g.: #( 1 3 nil 4 5 nil nil ) readStream next: 10.
-- Sven Van Caekenberghe http://stfx.eu Smalltalk is the Red Pill
-- Best regards, Igor Stasenko.
In other Smalltalk (VW, Dolphin, ...) there is nextAvailable: anInteger "Answer the next anInteger elements of the receiver. If there are not enough elements available, answer a collection of as many as are available." while next: would raise an Error if incomplete... Maybe it's time to adopt it. Nicolas 2013/3/21 Igor Stasenko <siguctua@gmail.com>:
On 21 March 2013 20:48, Sven Van Caekenberghe <sven@stfx.eu> wrote:
Hi,
Related to a hard to find Zinc bug that I just discovered, I require some feedback as to the semantics of SocketStream>>#next:
IMHO, the current implementation is semantically wrong. I would expect the semantics to be: try to read count elements and return a new species collection containing them. If less elements are available, return a smaller collection. ( It then becomes effectively a limited #upToEnd).
I always implemented #next: by delegating to #next:into:, to #next:into:startingAt: and finally #readInto:startingAt:count: (see for example ZdcAbstractSocketStream, ZnLimitedReadStream or ZnChunkedReadStream).
When you look at the implementations of #next:into:, #next:into:startingAt: and #readInto:startingAt:count: on SocketStream, the fact that ConnectionClosed is not signalled is clearly stated. Yet SocketStream>>#next: is different, it does signal ConnectionClosed, which makes it impossible (unless you proceed the exception) to read up to end. And this is what InflateStream>>#getFirstBuffer expects. And which fails when less than 65536 bytes are available. Note that InflateStream reads its subsequent buffers using #next:into:startingAt: (see #moveSourceToFront) so that it properly reads up to end without signalling ConnectionClosed for larger payloads.
I can easily program around it, but I think SocketStream>>#next: is wrong and should be changed.
Any opinions ?
Sven
I agree, the implementation notes of #next: should be: - read the N elements from receiver, answer a collection of read elements. if receiver at end, or will reach end before reading all N elements, answer a collection of elements which were successfully read. .. the caller can always check the collection size, and if it < N, (and he insists on having N) he can either throw an error or handle it more graciously.
'' readStream next: 5 answering '' so, it actually consistent with what i stating.
A reference implementation (in Stream), however doesn't works like that:
next: anInteger "Answer the next anInteger number of objects accessible by the receiver."
| aCollection | aCollection := OrderedCollection new. anInteger timesRepeat: [aCollection addLast: self next]. ^aCollection
this code will always answer collection of N elements.. the bad side of it, that if stream reaching end, the tail will be filled with 'userful' nils. :)
so , based on ReadStream implementation we can assume that caller already expects to receive <=N elements, but not N with partly filled with nils.
Because then you cannot distinguish whether this nil goes from stream data or from stream itself, e.g.:
#( 1 3 nil 4 5 nil nil ) readStream next: 10.
-- Sven Van Caekenberghe http://stfx.eu Smalltalk is the Red Pill
-- Best regards, Igor Stasenko.
Andrei, Igor, Nicolas, I just changed the implementation to SocketStream>>next: next: anInteger ^ self nextInto: (self streamBuffer: anInteger) and all NetworksTests-* and Zinc/Zdc tests are still green. @Andrei: could you please verify that this fixes your actual problem ? @Igor: I agree ;-) @Nicolas: Part of the problem is that the current implementors of #next: are inconsistent with each other, which is enough of a problem already. Looking at FileStream > StandardFileStream > MultiByteFileStream, they are inconsistent, for example. #nextAvailable: says better what we mean, agreed. But the 'available' word is not used anywhere else either. As Igor showed, how can the strange #next: behavior be useful ? I am more for simplifying what we have, instead of adding more selectors. And I think we all agree the current situation is not perfect, and more beautiful streams lurk in the future. Sven On 21 Mar 2013, at 22:45, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com> wrote:
In other Smalltalk (VW, Dolphin, ...) there is
nextAvailable: anInteger "Answer the next anInteger elements of the receiver. If there are not enough elements available, answer a collection of as many as are available."
while next: would raise an Error if incomplete...
Maybe it's time to adopt it.
Nicolas
2013/3/21 Igor Stasenko <siguctua@gmail.com>:
On 21 March 2013 20:48, Sven Van Caekenberghe <sven@stfx.eu> wrote:
Hi,
Related to a hard to find Zinc bug that I just discovered, I require some feedback as to the semantics of SocketStream>>#next:
IMHO, the current implementation is semantically wrong. I would expect the semantics to be: try to read count elements and return a new species collection containing them. If less elements are available, return a smaller collection. ( It then becomes effectively a limited #upToEnd).
I always implemented #next: by delegating to #next:into:, to #next:into:startingAt: and finally #readInto:startingAt:count: (see for example ZdcAbstractSocketStream, ZnLimitedReadStream or ZnChunkedReadStream).
When you look at the implementations of #next:into:, #next:into:startingAt: and #readInto:startingAt:count: on SocketStream, the fact that ConnectionClosed is not signalled is clearly stated. Yet SocketStream>>#next: is different, it does signal ConnectionClosed, which makes it impossible (unless you proceed the exception) to read up to end. And this is what InflateStream>>#getFirstBuffer expects. And which fails when less than 65536 bytes are available. Note that InflateStream reads its subsequent buffers using #next:into:startingAt: (see #moveSourceToFront) so that it properly reads up to end without signalling ConnectionClosed for larger payloads.
I can easily program around it, but I think SocketStream>>#next: is wrong and should be changed.
Any opinions ?
Sven
I agree, the implementation notes of #next: should be: - read the N elements from receiver, answer a collection of read elements. if receiver at end, or will reach end before reading all N elements, answer a collection of elements which were successfully read. .. the caller can always check the collection size, and if it < N, (and he insists on having N) he can either throw an error or handle it more graciously.
'' readStream next: 5 answering '' so, it actually consistent with what i stating.
A reference implementation (in Stream), however doesn't works like that:
next: anInteger "Answer the next anInteger number of objects accessible by the receiver."
| aCollection | aCollection := OrderedCollection new. anInteger timesRepeat: [aCollection addLast: self next]. ^aCollection
this code will always answer collection of N elements.. the bad side of it, that if stream reaching end, the tail will be filled with 'userful' nils. :)
so , based on ReadStream implementation we can assume that caller already expects to receive <=N elements, but not N with partly filled with nils.
Because then you cannot distinguish whether this nil goes from stream data or from stream itself, e.g.:
#( 1 3 nil 4 5 nil nil ) readStream next: 10.
-- Sven Van Caekenberghe http://stfx.eu Smalltalk is the Red Pill
-- Best regards, Igor Stasenko.
On 22 Mar 2013, at 15:34, Sven Van Caekenberghe <sven@stfx.eu> wrote:
SocketStream>>next: next: anInteger ^ self nextInto: (self streamBuffer: anInteger)
Make that SocketStream>>next: count "Read count elements and return them in a collection. If the receiver is #atEnd before count elements were read, return a smaller collection." ^ self nextInto: (self streamBuffer: count)
Hi Sven, In my case it solves the problem. Everything is working ok now. Thanks for the fix. Cheers, Andrei On Fri, Mar 22, 2013 at 4:04 PM, Sven Van Caekenberghe <sven@stfx.eu> wrote:
On 22 Mar 2013, at 15:34, Sven Van Caekenberghe <sven@stfx.eu> wrote:
SocketStream>>next: next: anInteger ^ self nextInto: (self streamBuffer: anInteger)
Make that
SocketStream>>next: count "Read count elements and return them in a collection. If the receiver is #atEnd before count elements were read, return a smaller collection."
^ self nextInto: (self streamBuffer: count)
On 22 Mar 2013, at 18:21, Andrei Vasile Chis <chisvasileandrei@gmail.com> wrote:
Hi Sven,
In my case it solves the problem. Everything is working ok now.
That is great news. Thank you, for reporting the issue, being persistent and helping us, this is an essential step for making progress in open source projects.
Thanks for the fix.
Cheers, Andrei
On Fri, Mar 22, 2013 at 4:04 PM, Sven Van Caekenberghe <sven@stfx.eu> wrote:
On 22 Mar 2013, at 15:34, Sven Van Caekenberghe <sven@stfx.eu> wrote:
SocketStream>>next: next: anInteger ^ self nextInto: (self streamBuffer: anInteger)
Make that
SocketStream>>next: count "Read count elements and return them in a collection. If the receiver is #atEnd before count elements were read, return a smaller collection."
^ self nextInto: (self streamBuffer: count)
+1 I'm all in for fixing and getting a simpler and nicer system.
#nextAvailable: says better what we mean, agreed. But the 'available' word is not used anywhere else either.
As Igor showed, how can the strange #next: behavior be useful ?
I am more for simplifying what we have, instead of adding more selectors.
And I think we all agree the current situation is not perfect, and more beautiful streams lurk in the future.
Sven
On 21 Mar 2013, at 22:45, Nicolas Cellier <nicolas.cellier.aka.nice@gmail.com> wrote:
In other Smalltalk (VW, Dolphin, ...) there is
nextAvailable: anInteger "Answer the next anInteger elements of the receiver. If there are not enough elements available, answer a collection of as many as are available."
while next: would raise an Error if incomplete...
Maybe it's time to adopt it.
Nicolas
2013/3/21 Igor Stasenko <siguctua@gmail.com>:
On 21 March 2013 20:48, Sven Van Caekenberghe <sven@stfx.eu> wrote:
Hi,
Related to a hard to find Zinc bug that I just discovered, I require some feedback as to the semantics of SocketStream>>#next:
IMHO, the current implementation is semantically wrong. I would expect the semantics to be: try to read count elements and return a new species collection containing them. If less elements are available, return a smaller collection. ( It then becomes effectively a limited #upToEnd).
I always implemented #next: by delegating to #next:into:, to #next:into:startingAt: and finally #readInto:startingAt:count: (see for example ZdcAbstractSocketStream, ZnLimitedReadStream or ZnChunkedReadStream).
When you look at the implementations of #next:into:, #next:into:startingAt: and #readInto:startingAt:count: on SocketStream, the fact that ConnectionClosed is not signalled is clearly stated. Yet SocketStream>>#next: is different, it does signal ConnectionClosed, which makes it impossible (unless you proceed the exception) to read up to end. And this is what InflateStream>>#getFirstBuffer expects. And which fails when less than 65536 bytes are available. Note that InflateStream reads its subsequent buffers using #next:into:startingAt: (see #moveSourceToFront) so that it properly reads up to end without signalling ConnectionClosed for larger payloads.
I can easily program around it, but I think SocketStream>>#next: is wrong and should be changed.
Any opinions ?
Sven
I agree, the implementation notes of #next: should be: - read the N elements from receiver, answer a collection of read elements. if receiver at end, or will reach end before reading all N elements, answer a collection of elements which were successfully read. .. the caller can always check the collection size, and if it < N, (and he insists on having N) he can either throw an error or handle it more graciously.
'' readStream next: 5 answering '' so, it actually consistent with what i stating.
A reference implementation (in Stream), however doesn't works like that:
next: anInteger "Answer the next anInteger number of objects accessible by the receiver."
| aCollection | aCollection := OrderedCollection new. anInteger timesRepeat: [aCollection addLast: self next]. ^aCollection
this code will always answer collection of N elements.. the bad side of it, that if stream reaching end, the tail will be filled with 'userful' nils. :)
so , based on ReadStream implementation we can assume that caller already expects to receive <=N elements, but not N with partly filled with nils.
Because then you cannot distinguish whether this nil goes from stream data or from stream itself, e.g.:
#( 1 3 nil 4 5 nil nil ) readStream next: 10.
-- Sven Van Caekenberghe http://stfx.eu Smalltalk is the Red Pill
-- Best regards, Igor Stasenko.
participants (5)
-
Andrei Vasile Chis -
Igor Stasenko -
Nicolas Cellier -
stephane ducasse -
Sven Van Caekenberghe