387588 - [FIX]Binary input stream and fastload read should handle short reads

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

I have a locally hacked nsBufferedStream which returns more short reads, and it looks like fastload can't deal.  I was getting bug 379491 as a result.

Comment 1

[Brendan Eich [:brendan]]

Comment on attachment 271719 [details] [diff] [review]
Fix

Looks good to me, if biesi agrees.

/be

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

considering that you can still get short reads (when rv == WOULD_BLOCK), how is this any improvement? and why shouldn't the caller take care of handling this?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> considering that you can still get short reads (when rv == WOULD_BLOCK)

Actually, for fastload you don't -- it's always reading from a file, with a buffered stream around it.  The improvement is that with this patch I can actually fastload things; without it I get errors because number of bytes read doesn't match expected number...

Note that I'd basically need to rewrite all of nsBinaryInputStream and all consumers to handle WOULD_BLOCK.  I suppose I can do that, but not for 1.9, and I'd like to fix the thing that actually has visible effects.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, but can't you make fastload handle short reads, instead of doing that here? the input stream contract allows this...

Comment 5

[Boris Zbarsky [:bzbarsky]]

> ok, but can't you make fastload handle short reads

I could stick this exact loop in every single caller of this method (all the Read* functions in nsBinaryInputStream).  But how would that be better?

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 271719 [details] [diff] [review]
Fix

I didn't realize the callers were inside of nsBinaryInputStream. perhaps you should mention that in this comment:
+    // mInputStream might give us short reads, so deal with that.

because it's not obvious why this function has to deal with it at all.

Why change ReadSegments? And anyway, that change is incorrect. If the reader function returns an error, you shouldn't call it again. Note that returning an error the first time the reader is called is a convenient way to look at data in the stream without consuming it.

Comment 7

[Brendan Eich [:brendan]]

We should not change fastload code to deal with short reads. It knows it has a local file. Is there any OS on which local file read would ever return short to avoid blocking (other than due to running up against eof)?

/be

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Why change ReadSegments?

Because nsBinaryInputStream::ReadCString and nsBinaryInputStream::ReadString call it.

I'll add checking for the reader function returning an error.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Test for reader errors

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #7)
> We should not change fastload code to deal with short reads. It knows it has a
> local file. 

It actually has a buffered stream. If I'm reading this bug correctly that's what causes the short reads here.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> (In reply to comment #7)
> > We should not change fastload code to deal with short reads. It knows it has a
> > local file. 
> 
> It actually has a buffered stream.

That's not meant to abstract over other than local file i/o, which was my point. I used it because I wanted buffering and didn't want to reinvent the wheel. I do regret how COMtaminated the fastload code is, and I hope to deCOMtaminate it hard soon.

> If I'm reading this bug correctly that's what causes the short reads here.

Yeah, that's unfortunate -- my mistake from 2001.

/be

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 272438 [details] [diff] [review]
Test for reader errors

sorry, forgot to mention that before, but you're passing a wrong aToOffset to the reader in the cases where you call ReadSegments more than once.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I am?  Why?  The aToOffset is whatever the underlying stream is passing to the reader; the fact that I'm calling ReadSegments on the underlying stream multiple times should not matter, since those calls don't pass in anything that maps to aToOffset.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

so the docs sort of suck, but the semantics are:
- aFromSegment points to the current piece of data
- aToSegment is basically the sum of all previous *aWriteCount values

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Better docs, better code. Third time might be the charm?

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 272594 [details] [diff] [review]
Better docs, better code.  Third time might be the charm?

oh, also, why reuse aCount and substract from it, but not reuse aBuffer? (in Read)

  * @param aWriteCount number of bytes read

perhaps mention that this is an out parameter? or is that obvious enough?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> oh, also, why reuse aCount and substract from it, but not reuse aBuffer?

Oops.  Relic from an earlier version of this code.  I'll reuse aBuffer in what I land.

And I'll add a comment.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Checked in.  I guess testing would involve a stream class that generates short reads on demand...