Undo BufferedIO#rbuf_consume_all_shareable! optimization #19
Conversation
This optimization is unsafe because `dest` is allowed to be a custom object responding to `<<` (e.g. a block wrapped in `ReadAdapter`). So the receiver can hold onto the passed buffer for as long as it wants. If it was guaranteed that `ReadAdapter` was the only possible receiver we could dup the buffer there for mutation safety, but I'm not certain it's the case so I'd rather err on the safe side. Ref: shrinerb/shrine#610
|
NB: I tried to write a test case for this, but it's very hard, as you need I'll try again tomorrow, but figured it was worth pushing the fix even without a test. |
|
@casperisfine Thanks a lot. I will release 0.2.1 because this issue caused data corruption in production. |
Unfortunately we have to use a mock, but this test demonstrate the mutation bug fixed in ruby#19. It fails on 0.2.0 but passes on 0.1.3 or 0.2.1.
|
Too bad the optimization didn't work, I was looking forward to it -- I have noticed that streaming large files via net-protocol can result in enormous RAM/GC/performance costs, even when you are just streaming from one IO to another and not buffering the whole thing. Due to creation of so many objects, I think. And I was hoping this optimization would help. But I actually use it via shrine anyway, so in fact it did not. I appreciate the quick response! |
|
Some part of the optimization it still there, it's not all lost. |
This optimization is unsafe because
destis allowed to be a custom object responding to<<(e.g. a block wrapped inReadAdapter).So the receiver can hold onto the passed buffer for as long as it wants.
If it was guaranteed that
ReadAdapterwas the only possible receiver we could dup the buffer there for mutation safety, but I'm not certain it's the case so I'd rather err on the safe side.Fix: #14
Ref: shrinerb/shrine#610
cc @ko1