SocketStream>>#next:

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

SocketStream>>#next:

Sven Van Caekenberghe-2
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


Reply | Threaded
Open this post in threaded view
|

Re: SocketStream>>#next:

Igor Stasenko
On 21 March 2013 20:48, Sven Van Caekenberghe <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: SocketStream>>#next:

Nicolas Cellier
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 <[hidden email]>:

> On 21 March 2013 20:48, Sven Van Caekenberghe <[hidden email]> 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.
>

Reply | Threaded
Open this post in threaded view
|

Re: SocketStream>>#next:

Sven Van Caekenberghe-2
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 <[hidden email]> 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 <[hidden email]>:
>> On 21 March 2013 20:48, Sven Van Caekenberghe <[hidden email]> 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.
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: SocketStream>>#next:

Sven Van Caekenberghe-2

On 22 Mar 2013, at 15:34, Sven Van Caekenberghe <[hidden email]> 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)



Reply | Threaded
Open this post in threaded view
|

Re: SocketStream>>#next:

Andrei Chis
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 <[hidden email]> wrote:

>
> On 22 Mar 2013, at 15:34, Sven Van Caekenberghe <[hidden email]> 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)
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: SocketStream>>#next:

Sven Van Caekenberghe-2

On 22 Mar 2013, at 18:21, Andrei Vasile Chis <[hidden email]> 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 <[hidden email]> wrote:
>>
>> On 22 Mar 2013, at 15:34, Sven Van Caekenberghe <[hidden email]> 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)
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: SocketStream>>#next:

stephane ducasse
In reply to this post by Sven Van Caekenberghe-2
+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 <[hidden email]> 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 <[hidden email]>:
>>> On 21 March 2013 20:48, Sven Van Caekenberghe <[hidden email]> 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.
>>>
>>
>
>