Hi,

I'm often reading fixed-size structures using TStream.ReadBuffer, working under the assumption that it will fail only if it's not possible to read the requested amount of bytes (that is, if the stream ended prematurely). This is actually the documented usage, see http://www.freepascal.org/docs-html/rtl/classes/tstream.readbuffer.html and see many usage examples of ReadBuffer in FPC, Lazarus etc. sources. I'm using various stream classes in my code (TFileStream, TGZFileStream, TMemoryStream, TBase64DecodingStream...) and I always depend that ReadBuffer will work correctly for all streams.

But in some hardly-reproducible cases sometimes ReadBuffer fails even though I know I have more bytes. Looking at the sources, I was surprised to see this ReadBuffer implementation (in rtl/objpas/classes/streams.inc ):

procedure TStream.ReadBuffer(var Buffer; Count: Longint);
     begin
        if Read(Buffer,Count)<Count then
          Raise EReadError.Create(SReadError);
     end;

This is quite incorrect, IMHO. The TStream.Read implementations of various streams can sometimes return less Count than requested, and that's Ok, and that doesn't mean that stream ended. Stream ended only when TStream.Read returns exactly zero.

Which means that ReadBuffer should retry calling Read, in a loop, trying to finish the reading. ReadBuffer should raise an exception only when Read returns zero (before we can gather all the Count bytes).

Right now, the way I see it, a lot of the code using ReadBuffer works purely by accident. At least if it's supposed to work with any TStream descendant.

For example: on Linux, TFileStream.Read calls fpRead which is a system call, and it definitely can return less than requested amount of bytes, and it doesn't mean that stream ended. (See e.g. Libc docs: http://www.gnu.org/software/libc/manual/html_mono/libc.html#I_002fO-Primitives : "If read returns at least one character, there is no way you can tell whether end-of-file was reached. But if you did reach the end, the next read will return zero. ") So if you use TFileStream with ReadBuffer, your code is working purely by accident right now...

If you agree, I can provide a patch to ReadBuffer (and similar to WriteBuffer).

Michalis

Michalis Kamburelis wrote

Which means that ReadBuffer should retry calling Read, in a loop, trying to finish the reading. ReadBuffer should raise an exception only when Read returns zero (before we can gather all the Count bytes).

And what with non-blocking pipes pipes? Wait for a *some* period until you get all data? It is up to the programmer to do this INHO. Also, if you would enforce this behaviour on all kinds of streams, you might (`will`, actually) get unexpected performance drops...

For example: on Linux, TFileStream.Read calls fpRead which is a system call, and it definitely can return less than requested amount of bytes, and it doesn't mean that stream ended. (See e.g. Libc docs: http://www.gnu.org/software/libc/manual/html_mono/libc.html#I_002fO-Primitives : "If read returns at least one character, there is no way you can tell whether end-of-file was reached. But if you did reach the end, the next read will return zero. ") So if you use TFileStream with ReadBuffer, your code is working purely by accident right now...

But if you work with a blocking fd (which, for example, a TFileStream uses IIRC) you will always get your data. If there is more data to be read(), but it would take time, it simply takes time. The only case where zero is returned, is IIRC (like you quoted) when the end of the `fd` (e.g. pipe, socket, file) is reached on *blocking filedescriptors*.

Note that the above is a rather sumarized explanation, but it should make the point.

FYI: I've never had the problem of a `partial read` (e.g. SomeStream.Read(50) returning 36 or something) on linux, osx and windows; so perhaps you have exposed some bug in an obscure TStream descendant?

Ewald wrote

And what with non-blocking pipes pipes? Wait for a *some* period until you get all data? It is up to the programmer to do this INHO.

If you want to get partial data (instead of waiting until all requested data is available) you should just use TStream.Read instead of TStream.ReadBuffer. Then it's indeed up to the programmer to handle this.

All existing usage of ReadBuffer right now works only if you get all the data in a single Read call (which translates to a single read syscall on Linux, as far as I see), otherwise it fails with exception. That's just wrong behavior for a ReadBuffer IMHO.

Also, if you would enforce this behaviour on all kinds of streams, you might (`will`, actually) get unexpected performance drops...

I don't see why you would get any performance drop. If you're lucky and a single Read call is all you need, then the speed is the same as current implementation, since the 1st check "if Read(Buffer,Count)<Count then" will return false and we will not call Read again.

Besides, we're talking about correctness here. And TStream.ReadBuffer is non-virtual and must be suitable for all TStream descedants, handling whatever virtual TStream.Read does.

Yes, zero is returned only if stream ends. But a result that is non-zero, but still less than requested Count, doesn't tell you if the stream ends. The whole point is whether ReadBuffer should take this into account.

Reproducing this behavior proved to be difficult, but I definitely observed it with TFileStream on Linux.

And, even if it would accidentally work for some streams, that's not a solution. Like I mentioned, TStream.ReadBuffer is non-virtual and must be suitable for every TStream.

Michalis

Sorry, I misread your mail last night (it was getting late I believe), I thought you said `TStream.Read` instead of `TStream.ReadBuffer`. In this case you are probably right.

Never mind, my brain was still thinking on the wrong call here. Sorry for the noise there :-)

If the `ReadBuffer` call uses `Read` to read the data (which would be rather obvious) than yes, it would need to take this into account as it cannot know which subsystem `Read` uses.

Did it got interrupted by a signal then? Or perhaps the `Count` argument of `ReadBuffer` was greater than SSIZE_MAX? Either way, as ReadBuffer gives no information about the number of bytes actually read, it should indeed `try harder`.

And, even if it would accidentally work for some streams, that's not a solution. Like I mentioned, TStream.ReadBuffer is non-virtual and must be suitable for every TStream.

The question that arises here is whether all the `ReadByte`, `ReadWord`, `WriteByte`, `WriteWord`, `WriteBuffer`, ... should also `try harder`? Same issue there: no data is returned about the actual number of bytes read. Anyway, I leave that to someone who knows the internals of these classes.

The above implementation should not be changed, it is Delphi compatible: TStream.ReadBuffer works on the assumption that Read will always return the amount of bytes requested if they are available.

So, if you want to make changes, there is no reason why TStream.Read should not benefit from 'trying harder', it could try harder to actually get the requested number of bytes from the underlying low-level implementation. Some TStream descendents already work that way (compression, hash, encoding), others don't.

This in turn means that affected TStream descendents such as TFileStream or THandleStream should try harder to read the requested number of bytes, taking into account the specifics of the underlying file/socket/whatever protocol.

Michael.

ReadByte, WriteByte and friends already use ReadBuffer / WriteBuffer underneath. E.g. TStream.ReadByte simply calls ReadBuffer(b,1). So fixing ReadBuffer / WriteBuffer would automatically also fix them :)

Streams are implemented in rtl/objpas/classes/streams.inc in FPC sources.

Michalis

In effect, you want to change all the streams Read method implementation, such that it returns less than requested Count *only* when the stream actually ended...

What about socket streams that potentially download the data? Or THandleStream streams that *may* read/write data from/to a pipe, like stdin/stdout, and may have to wait for another process? Should their TStream.Read really hang, waiting for Count data?

1. See Ewald objections. Like Ewald, I also think it is useful to have a TStream.Read that only returns the amount of data that is immediately available (and hangs only when no data is available yet).

2. This would be large work, IMHO... I propose to fix a single TStream.ReadBufffer implementation (well, and TStream.WriteBuffer). You propose to fix Read method in all stream descendants, inside and outside FPC sources.

Michalis

Yes.

If an implementation of Read does not return the requested data when in fact it is available, then the implementation of Read is broken.

Your proposal is a quick and dirty hack that does not address the real problem.

Once more, ReadBuffer operates under the assumption that Read returns all requested data. That is what it does: it checks that read returns the requested data.

Michael.

Michael Van Canneyt wrote

If an implementation of Read does not return the requested data when in fact it is available, then the implementation of Read is broken.

I guess it depends how do you interpret TStream.Read documentation... http://www.freepascal.org/docs-html/rtl/classes/tstream.read.html does not explicitly say that TStream.Read returns less than requested Count *only when the stream ends*. And I saw a lot of code working under the assumption that only when TStream.Read returned zero -> then the stream ended.

If you want to put stricter requirements on TStream.Read, I suggest to update the docs to explicitly say this.

Your proposal is a quick and dirty hack that does not address the real problem.

Whether it's a hack or not really depends on your definition of "correct TStream.Read implementation".

My proposal is also a single solution that will make ReadBuffer correct for all the eternity.

Your proposal means that we have to fix THandleStream (Read and Write), which is definitely broken according to your requirements. Then we check TGZFileStream (I'm not sure whether it's already correct; it is definitely incorrect if gzRead in PasZLib is equivalent to gzRead inside C Zlib). And then every time I use another TStream implementation I'll have to check whether it's Read method is trying hard enough. TStream.Read that doesn't try hard enough is a very sneaky bug, since it will often work by accident (if reading small enough data).

Michalis

In my opinion, if you ask Count bytes, TStream should try to get the requested number of bytes, only failing if they really are not available.

Meanwhile, someone pointed out to me that at least in Delphi XE2, TStream.ReadBuffer has been changed to do a loop anyway. I guess we'll have to follow suit, even if I don't think it is correct :(

So please post a bugreport (with or without) patch on the bugtracker, you can assign it to me.

Michael.