Error incomplete while reading stamp frame

[bocasti]

You were right, I should first do the pull request to the origin repo, which is this one.
Sorry, I am on my way to be used to the contribution process :)

The intention of this PR is to handle properly an unexpected state, that has been experimented:
"the socket stream is not at end, but reading the next byte answers nil and raise errorIncomplete".

Here some more info about the case I think we faced:

"Ok I think I see a bit more precisely what can be the scenario for this "unexpected" state:
When sending "next" to the socket stream, and the buffer is empty,
the expected behavior of the "next" method is to wait for data.

As soon as it get some, it answers.
EXCEPT when the connection close:

fillReadBuffer

"Ask the socket to fill the read buffer with data. Wait for a data."
self fillReadBufferNoWait.
(readBuffer isEmpty and: [ self isConnected ])
	ifTrue: [ 
		[ self socketWaitForData ] on: ConnectionClosed do: [ ^ self ]. "when successful, recurse, else signal exception"
		self fillReadBuffer ]  

So if the connection close while "next" method is waiting for data, ConnectionClosed exception will be ignored , and nil will be answered.
Then, patatra"

[bocasti]

I wondering if you saw this PR as I was a bit messy with my previous one ? :)

[svenvc]

I did see this issue and it is on my todo list.

But I am not yet sure what to do or how to react.

I am very hesitant to change code in this area (let's call it socket stream behaviour related to network states).

Do you have a reproducible case ?

In my opinion, #next is ill specified in Pharo: it returns the next available item, it blocks, but it can both throw an exception as well as return nil - for example, a stream that is #atEnd will return nil for #next, while it there could also be an StreamEnded exception.

Your code is also written to assume lots of internal knowledge, which is never a good sign. But of course, the issue must be fixed somehow.

[bocasti]

Ok, thanks for answering !

Unfortunately, I don't have a reproducible case.
This error seems to occurs a bit randomly, especially when some update / deployment tasks are being made on the server.
which would strengthen our hypothesis of something going wrong while handling a closed connection.

I am myself also not completely satisfied with my changes.

I would see the error context as 3 different layers interacting together.
from the lower to the higher:

1. The socket layer, managing the raw incoming data (Zinc/Zodiac packages)
2. The stamp layer, modeling on top of the received data (Stamp packages).
3. The application layer, providing some logic on top of the received stamp frames (Our application packages)

And as you, the lower we are, the less I would question about something wrong,
and the more I would hesitate to actually change something :)

The changes I pushed with this PR are intended to enhance robustness in the second layer,
while manipulating the first layer.
But indeed, they are assuming some deep knowledge of the first layer.
At the end, I am still curious to understand why a ConnectionClosed occuring in the first layer would be ignored (as in #fillReadBuffer). As the lowest level, I guess there might be some sensitive concerns.

So I understand it is not a straight forward issue that maybe needs time to be thought about.
What I could also do in the meantime, is to identify the generated exceptions on the third layer (#errorIncomplete), and try to workaround it.

[svenvc]

"At the end, I am still curious to understand why a ConnectionClosed occuring in the first layer would be ignored (as in #fillReadBuffer). As the lowest level, I guess there might be some sensitive concerns."

It is not really ignored as far as I can see. Suppose you read 1k bytes while filling the buffer and then ConnectionClosed happens because the server says he's done. Then the higher layers still need the 1b bytes read previouslty, and only then the stream will be atEnd.

But yes, maybe the line reader is not 100% OK, there are other line readers in the image (at least 2 in Zn), maybe we need to compare the code.

[bocasti]

Ok, then I understand the concern, thank you.

Following this logic, maybe the #readLine method from StampMedium should consider it has reach the end when such an errorIncomplete occurs while waiting for the next character.
And answers what has been read before facing the exception.

[svenvc]

Have a look at ZnLineReader>>#processNext and ZnFastLineReader>>#nextLine both are taking #atEnd and #next isNil into account, we should try to add that here as well, probably.

[svenvc]

OK, what about

StampMedium>>#readLine
	^ self stringStreamContents: [ :out | | endOfLine |
		endOfLine := false.
		[ stream atEnd or: [ endOfLine ] ] whileFalse: [ | char |
			out position >= StampConstants maxHeaderLineLength
				ifTrue: [ self error: 'Line too long' ]. 
			(char := [ encoder nextFromStream: stream ] on: ZnIncomplete do: [ nil ])
				ifNil: [ endOfLine := true ]
				ifNotNil: [
					char = Character cr
						ifFalse: [ 
							char = Character lf
								ifTrue: [ endOfLine := true ]
								ifFalse: [ out nextPut: char ] ] ] ] ]

?

[bocasti]

Yes! Would be consistent with the other methods you pointed out.

[bocasti]

I just pushed a new commit that implements this solution.

Side issue: this file appear in the changes whereas I did not intentionally make any change from pharo:

repository/Stamp.package/TStampFrameWithBody.trait/properties.json

Could it be an issue around "pharo git format" and how traits are represented ?