fix incorrect Read calls #4792
fix incorrect Read calls #4792
Conversation
ghost
assigned 
commands/channelmarshaler.go
Outdated
| cr.reader = nil | ||
| err = nil |
There was a problem hiding this comment.
Are you sure we want to nil out the error in this case and not propagate the io.EOF? This is Read method after all.
There was a problem hiding this comment.
Had to think about that for a moment... yes. We're concatenating output so this is the continue case. The next time the user calls Read, we'll hit the cr.reader == nil case. If there are no more readers, we'll return the EOF.
There was a problem hiding this comment.
Doesn't this create the case @kevina described in #4793 (comment) (where we return nothing and no error)?
There was a problem hiding this comment.
Good point. We should probably loop.
unixfs/io/pbdagreader.go
Outdated
| if err != io.EOF { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
@Stebalien what do you think about using io.ReadFull or io.ReadAtLeast here. They will return io.ErrUnexpectedEOF or io.EOF if EOF is hit before the buffer is full and thus telling us to load the next buffer.
This would decrease significantly complexity of this function.
For context: https://golang.org/src/io/io.go?s=10738:10804#L293
There was a problem hiding this comment.
I considered it but then I got lazy. However, you are right. It'll probably make this simpler.
There was a problem hiding this comment.
So, this function actually needs to read multiple files so using ReadFull ends up making it more complex.
|
@Stebalien I've updated it to use io.ReadFull. Your changes to the dagmodifier seem to cause tests to fail. |
|
Just confirmed and it is the 1296cda |
Fixed. |
|
I will squash it to correct commit, so we won't have broken commit in middle of tree. Edit: fixed gramma |
Fair enough. I just didn't want to hide the change. |
Kubuxu
added
need/review
commands/channelmarshaler.go
Outdated
| cr.reader = nil | ||
| err = nil |
There was a problem hiding this comment.
Doesn't this create the case @kevina described in #4793 (comment) (where we return nothing and no error)?
Kubuxu
added
RFM
and removed
status/in-progress
|
@Stebalien can i get a rebase here? |
The dagmodifier *only* works because we're using a bytes.Buffer. License: MIT Signed-off-by: Steven Allen <[email protected]>
License: MIT Signed-off-by: Steven Allen <[email protected]>
* don't assume that Read fills the buffer. * don't succeed if the API file is too large. License: MIT Signed-off-by: Steven Allen <[email protected]>
License: MIT Signed-off-by: Steven Allen <[email protected]>
License: MIT Signed-off-by: Jakub Sztandera <[email protected]>
|
Rebased |
follow up on #4790
review commit by commit
Note: I ignored all test cases.