Always read unbuffered when IO::Buffered#sync = true #5849
Conversation
| return nil unless sync? | ||
|
|
||
| byte = uninitialized UInt8 | ||
| if read(Slice.new(pointerof(byte), 1)) == 1 |
There was a problem hiding this comment.
byte if read(Slice.new(pointerof(byte), 1)) == 1? Than you could skip else branch altogether.
There was a problem hiding this comment.
I prefer my more explicit version.
There was a problem hiding this comment.
No. An else nil branch is always useless. Please drop it.
There was a problem hiding this comment.
Same for the return nil above. return always returns nil. Skip the noise.
There was a problem hiding this comment.
I thought we agreed to skip the pointless bikeshedding about style. This kind of shit just drives away contributors. Until someone puts in the effort for an official style guide, the formatter is the style guide. And this formats.
There was a problem hiding this comment.
Also note that I copied this directly from io.cr:
Lines 302 to 309 in 2d93603
There was a problem hiding this comment.
Formatter ain't a rewritter.
I never accepted the introduction of useless nils, and will always ask for them to be removed. Copying noise isn't a reason to keep it.
|
io.read(16) # => buffers at most 8192 bytes
io.sync = true
io.read_byte # => what's read? buffer or IO? |
spec/std/io/buffered_spec.cr
Outdated
|
|
||
| byte = Bytes.new(1) | ||
| io.read_fully(byte) | ||
| byte[0].should eq('b'.ord.to_u8) |
There was a problem hiding this comment.
Maybe check that once the buffer is exhausted it will properly resume reading from the IO?
There are 2 independent tests (empty buffer, previously buffered), but the switch isn't tested.
There was a problem hiding this comment.
Why not add some more bytes to the IO::Memory after the first buffered read?
There was a problem hiding this comment.
Oh, yeah thats a good idea.
spec/std/io/buffered_spec.cr
Outdated
| io.sync = true | ||
| io.sync?.should be_true | ||
|
|
||
| io.read_byte.should eq('b'.ord.to_u8) |
There was a problem hiding this comment.
Same here. Maybe even check for nil (EOF)?
|
@RX14 How it wasn't optimal? As far as I know, in Ruby |
|
See #5843. Read buffering sometimes isn't free or invisible - especially when working with systems-level programming - and there should absolutely be a way to turn it off. |
|
Should it also be individually configurable for each direction? |
Previously the sync option only affected writes, which wasn't optimal.
RX14
force-pushed
the
feature/io-buffered-sync-read
branch
2 times, most recently
from
369fcfb
to
a980e0b
Compare
RX14
force-pushed
the
feature/io-buffered-sync-read
branch
from
a980e0b
to
2817969
Compare
|
I've rebased this and improved the specs. @ysbaddaden is this fine now? If we add |
|
I'm fine with this, but @asterite said |
|
If we did that, i'd rather just have a |
|
This does fix a security bug though, so it'd be great to arrive on a decision pretty quick (before the next release). |
|
Let's get this shipped first 👍 |
|
So.... LGTM? having security holes is not that good. |
* Always read unbuffered when IO::Buffered#sync = true Previously the sync option only affected writes, which wasn't optimal. * fixup! Always read unbuffered when IO::Buffered#sync = true * fixup! Always read unbuffered when IO::Buffered#sync = true
Previously the sync option only affected writes, which wasn't optimal.