improve AsyncIO robustness
#687
Conversation
- Fix `unsafe_read` to correctly calculate remaining bytes (`nb - written`) and use proper destination offset (`to + written`) - Fix potential race condition in `peek` by checking buffer under lock Written with Claude.
serenity4
left a comment
serenity4
left a comment
There was a problem hiding this comment.
This seems to be 95% LLM noise. It was a bit hard to understand what it's supposed to do and why. Does it address the issues that you are observing? It would be nice to have a link to the original error to see what needs fixing/what to look for.
The addition on line 121 of checking that the task is still running in unsafe_read seems worth adding, but I would oppose the rest of the changes as it is.
src/testing.jl
Outdated
| written = 0 | ||
| written = UInt(0) | ||
| while written < nb | ||
| bytesavailable(io) > 0 || (yield(); continue) | ||
| @lock io.lock begin | ||
| n = @lock io.lock begin | ||
| (; buffer) = io | ||
| n = min(length(buffer), nb) | ||
| GC.@preserve buffer begin | ||
| unsafe_copyto!(to, pointer(buffer), n) | ||
| n = min(UInt(length(buffer)), nb - written) | ||
| if n > 0 | ||
| GC.@preserve buffer begin | ||
| unsafe_copyto!(to + written, pointer(buffer), n) | ||
| end | ||
| splice!(buffer, 1:n) | ||
| end | ||
| splice!(buffer, 1:n) | ||
| written += n | ||
| n | ||
| end | ||
| written += n | ||
| if written < nb | ||
| # Yield first to give the background task a chance to read more data | ||
| yield() | ||
| eof(io) && throw(EOFError()) |
There was a problem hiding this comment.
From what I can see, on a semantic level, the changes:
- perform multiple
unsafe_copyto!instead of waiting for the buffer to have all the data then copy in one go. - check for
eof(io)at the end of every iteration.
But nothing related to
Fix unsafe_read to correctly calculate remaining bytes (nb - written) and use proper destination offset (to + written)
and we should already correctly calculate the right number of bytes.
The rest seems to be mostly noise. If the issue was that we need an extra istaskdone(io.task) && throw(EOFError()) in the loop, I'm all good with it, but through the LLM noise that single line appears to be the only change worth noting and the rest just increases the complexity for no reason AFAIU.
|
Thanks for the review. Looks like I submitted a low-quality PR because I didn't properly review Claude's output. Sorry for that. I made these changes while debugging #684. Actually, About this PR, you're right, I wrote it randomly. I just thought it would be a straightforward fix and created this PR taking what Claude saying, but I should have reviewed these changes more carefully.. |
This reverts commit bfc4d0d.
aviatesk
changed the title
AsyncIO robustness
|
No harm done! Apologies if I may have sounded a bit salty in my comments. I ran the tests on my machine with Julia 1.12.2, and they all pass without any issues. If you ever happen to have a reproducer, or just see it fail on CI, I can take a look at potential issues. The whole point of this |
Co-authored-by: Cédric Belmant <[email protected]>
No, I totally get that it's annoying to review random code, whether it's AI-generated or not, so I'm just sorry I put you through that. |
- Fixunsafe_readto correctly calculate remaining bytes (nb - written) and use proper destination offset (to + written)- Fix potential race condition inpeekby checking buffer under lockWritten with Claude.
EDIT: Turns out I just ended up crafting noises. Picked up one minor improvement from it.