Possible FileStream and/or binary filer bug?

Andy/Blair,

First, I'll extend a provisional appology in case this turns out to be a
false alarm.  I was doing some writing this weekend, and on noticing some
inconsistencies in some of my code, fired up my bug tracking system to
document them - using it's cool new feature of dragging from browsers to add
items.  Since that feature is pretty cool, and even more new (read "not
fully regression tested"<g>), I have been very careful in using it, and
frequently close and re-open files to verify that they can be read so that I
won't archive garbage.

At one point, the file I had just saved didn't load!  I had the good sense
to save it (and if necessary can email it to you, as long as you promise not
to tell anybody just how many bugs I have in my software<g>).  However, it
looked to me as though the file was simply truncated when it was written.  I
restored a recent backup of the file and re-documented the offending items,
though using slightly different text; the resulting file was able to load.

A little debugging of the failed load revealed an 8k collection backing up
the stream, but, some experiments didn't reveal any obvious differences
between this and files that worked.  I noted that the troubled file was
"about 40k" in size, and went about the cautious use of the restored bug
datbase.

About an hour ago it hit me: what if the problem was that the failed file
somehow hit an integral multiple of the buffer size?  I wanted to keep the
binary filer involved, because I don't know where the problem occurs, so I
decided to get a byte array that binary filed to 40k and write/read a file.
It worked =:0   One more desperate attempt: I added one byte, and got an end
of stream error.

Here's the code:

| sizes fileName out b c |
#( 8168 8169 ) do:[ :n |
 b := ByteArray new:n.
 fileName := 'd:\stuff\teststb.stb'.
 out := FileStream write:fileName text:false.
 [
  b binaryStoreOn:out.

 ] ensure:[ out close. ].

 in := FileStream read:fileName text:false.
 [
  c := Object binaryReadFrom:in.
 ] ensure:[ in close. ].
 nil assert:[ c size = n ].
].

I'm finding that it works for the first size (writing 40k) but fails when
trying to read back the second blob, which writes 40k+1byte.  Again,
appologies if I'm doing something stupid; I wanted to report this right away
because it's kinda important if there really is a problem.

Have a good one,

Bill

--
Wilhelm K. Schwab, Ph.D.
[hidden email]

Bill,

It is a bug, and one that's been in there since V3 at least!!  That is even
more surprising since it is not the STB filer that is wrong but the
FileStream itself.

You can simplify it by avoiding the STB stuff -

n := 8193.
fs := FileStream write: 'test.bin' text: false.
[1 to: n do: [:each | fs nextPut: each \\ 256]] ensure: [fs close].

fs := FileStream read: 'test.bin' text: false.
[self assert: [fs contents size = n]] ensure: [fs close].

which answers one byte short (nb: it works for 8192 or 8194). The problem
starts around FileStream>>nextPut: which reads

flags := flags bitOr: DirtyBufferMask.
self primitiveNextPut: anIntegerOrCharacter

where you are setting the dirty flag before the primitive file output
operation. If the FileStream>>primitiveNextPut: needs to start a new buffer
for the write, as it does at the 8192-8193 boundary, then a #flush is
performed on the old buffer which automatically resets the dirtyFlag (see
FileStream>>flush). If you now immediately #close the file then the odd byte
that was just added is not flushed as the dirty flag is now clear and
FileStream>>flush therefore assumes it can ignore any further #flush
request.

A possible fix might be to invert the sequence in FileStream>>nextPut: to
the seemingly more logical

self primitiveNextPut: anIntegerOrCharacter.
flags := flags bitOr: DirtyBufferMask.
^anIntegerOrCharacter

but I'm not sure if this will break things _if_ the last byte written is the
last byte in a buffer (8192). I can't seem to make it fail but I didn't test
it exhaustively.

Ian

Hi Ian,

> It is a bug, and one that's been in there since V3 at least!!

I got the same results in 3.06 - it's hard to believe we've missed it all
this time.


> That is even
> more surprising since it is not the STB filer that is wrong but the
> FileStream itself.

I had suspected as much; thanks for confirming it.  Might there be a similar
problem in socket streams?


> which answers one byte short (nb: it works for 8192 or 8194). The problem
> starts around FileStream>>nextPut: which reads
>
> flags := flags bitOr: DirtyBufferMask.
> self primitiveNextPut: anIntegerOrCharacter
>
> where you are setting the dirty flag before the primitive file output
> operation. If the FileStream>>primitiveNextPut: needs to start a new
buffer
> for the write, as it does at the 8192-8193 boundary, then a #flush is
> performed on the old buffer which automatically resets the dirtyFlag (see
> FileStream>>flush). If you now immediately #close the file then the odd
byte
> that was just added is not flushed as the dirty flag is now clear and
> FileStream>>flush therefore assumes it can ignore any further #flush
> request.

Nice work - fast too :)


> A possible fix might be to invert the sequence in FileStream>>nextPut: to
> the seemingly more logical
>
> self primitiveNextPut: anIntegerOrCharacter.
> flags := flags bitOr: DirtyBufferMask.
> ^anIntegerOrCharacter
>
> but I'm not sure if this will break things _if_ the last byte written is
the
> last byte in a buffer (8192). I can't seem to make it fail but I didn't
test
> it exhaustively.

Thanks!

Bill

--
Wilhelm K. Schwab, Ph.D.
[hidden email]

Bill,

> I had suspected as much; thanks for confirming it.  Might there be a
> similar problem in socket streams?

It looks like there should be but in practice it doesn't seem to cause a
problem. I'm don't know why at the moment but I'm sure I shan't be able to
resist investigating <g>

> > but I'm not sure if this will break things _if_ the last byte written is
> > the last byte in a buffer (8192). I can't seem to make it fail but I
> > didn't test it exhaustively.

On further thought this shouldn't be a problem as the error only occurs when
you attempt to write the first byte into a new buffer.

Ian

> It looks like there should be but in practice it doesn't seem to cause a
> problem. I'm don't know why at the moment but I'm sure I shan't be able to
> resist investigating <g>

Forget that, SocketWriteStream is broken as well. I was fooled by thinking
that there was the same relationship between #nextPut: and #nextPutAll: in
SocketWriteStream as in a normal WriteStream - wrong!!

Evaluating the following, assuming a correct Socket connection, results in
the last byte being lost.

s := SocketWriteStream on: socketA.
n := 4097.
((1 to: n) collect: [:each | each \\ 256]) asByteArray do: [:each |
    s nextPut: each].
s flush

Editing SocketWriteStream>>nextPut: in the same manner as before solves the
problem.

FWIW, because #nextPutAll: works differently the following also sends all
the expected bytes but without the need for a change to the #nextPut:

s := SocketWriteStream on: socketA.
n := 4097.
s nextPutAll: ((1 to: n) collect: [:each | each \\ 256]) asByteArray.
s flush

Ian

"Ian Bartholomew" <[hidden email]> wrote in message
news:9becr0$e5s$[hidden email]...
>
> It is a bug, and one that's been in there since V3 at least!!  That is
even
> more surprising since it is not the STB filer that is wrong but the
> FileStream itself.

Thanks to Bill for finding the bug, and Ian for the detective work and the
fix (which looks to be correct at first glance). We'll patch this in PL2.

Regards

Blair