nio4r icon indicating copy to clipboard operation
nio4r copied to clipboard

Inconsistent end of stream handling in ByteBuffer::read_from

Open Komfr opened this issue 5 years ago • 4 comments

The three implementations of this method differ in how they handle end of stream:

  • Native implementation will throw TypeError exception because read_nonblock with exception set to false returns nil which gets passed to <<.
  • C extension returns 0 which is indistinguishable from 0 which it returns when EAGAIN error is returned so the caller is unable to differentiate between those two cases.
  • JRuby version seems to be throwing EOFError

Komfr avatar Jul 10 '20 15:07 Komfr

I guess the question here is: what should the behavior be? Throw EOFError? Return nil?

tarcieri avatar Jul 13 '20 16:07 tarcieri

While I am generally a fan of nil values, I think that EOFError exception would be the most appropriate thing here from the compatibility point of view. First it is already used by one version so robust code using this class is likely to be ready for it. Either explicitly based on knowledge of the implementation or generally by expecting this kind of exception to happen in IO related context. Such code might not need any update to be compatible with the new version unlike the nil result which would require some changes and can cause unexpected bugs if the change is not noticed by its maintainer. Second if the code is not ready, the exception will be easier to notice and fix than nil value which could be propagated to unexpected place and possibly cause some silent issue.

Komfr avatar Jul 13 '20 19:07 Komfr

IMHO, exceptions should be the exception - associated only with errors... and the EOF situation is not really an error, it's somewhat expected and sometimes even relied upon.

Moreover, exceptions incur a high performance cost, they are implemented by saving the stack and registers of one location and then (when the exception occurs) folding the stack back and "clobbering" and overwriting the CPU registers (some CPU registers actually have their own stack that needs to be reset).

I would suggest nil as the correct return type.

boazsegev avatar Sep 01 '20 09:09 boazsegev

That makes sense to me.

ioquatix avatar Sep 12 '20 10:09 ioquatix