Validate remote read byte counts - #981
Conversation
madsbk
added
improvement
|
/ok to test 164a360 |
| KVIKIO_CUDA_DRIVER_TRY(cudaAPI::instance().StreamSynchronize(stream)); | ||
| copied += chunk_size; | ||
| } | ||
| } |
There was a problem hiding this comment.
This change seems to needlessly introduce another level of intermediate bounce buffer. Why is it not possible to just use the existing setup and check the relevant error condition as part of curl.perform?
There was a problem hiding this comment.
Thank you for your attention, I have amended.
vuule
left a comment
vuule
left a comment
There was a problem hiding this comment.
C++ changes look good, just one suggestion
|
/ok to test a2d41e9 |
|
I will push ASAP. @vuule |
|
/ok to test f5fa2cf |
| curl.perform(); | ||
| curl.set_before_perform_attempt([&ctx, &bounce_buffer] { | ||
| detail::reset_callback_context(ctx); | ||
| bounce_buffer.reset(); |
There was a problem hiding this comment.
I am concerned here. This is before we're calling curl.perform(). How can we get a bounce buffer that is not reset?
| bounce_buffer.reset(); | ||
| throw; |
There was a problem hiding this comment.
OK, so if we get an exception then we should reset the bounce buffer before returning it to the cache?
It seems like that should happen in the dtor for bounce_buffer. Rather than having to remember to do this manually.
|
|
||
| try { | ||
| if (is_host_mem) { | ||
| curl.set_before_perform_attempt([&ctx] { detail::reset_callback_context(ctx); }); |
There was a problem hiding this comment.
I kind of think we should just inline the definition of these functions here. I don't think it buys us anything to introduce these detail functions.
Also, again, how are we managing to make a CallbackContext that is not in a reset state before the curl.perform() call? That seems like a bigger issue that we shouldn't paper over by calling reset before perform.
There was a problem hiding this comment.
Thanks, agreed. I updated this:
CallbackContext starts in the reset state by default.
Resetting now happens only through curl.perform(on_retry), before a retry, not before the first attempt.
The byte-count checks are inlined in the easy and multi-poll paths.
The device bounce buffer is reset on failure before its destructor runs, so partial data is not flushed.
I also removed the extra callback setter/helper functions.
4 participants
Summary
Why
A remote read could return success after libcurl completed even if the write callback received fewer bytes than requested. The easy retry path also reused the same callback offset between attempts, so a partial timed-out attempt could make the next attempt append at the wrong offset.
This keeps the existing host and device callback paths, restarts per-attempt byte state on retries, and rejects successful transfers that did not deliver the requested byte count.
Validation
pre-commit run --files cpp/include/kvikio/detail/remote_callback.hpp cpp/src/detail/remote_callback.cpp cpp/include/kvikio/shim/libcurl.hpp cpp/src/shim/libcurl.cpp cpp/src/remote_handle.cpp cpp/src/detail/multi_poll_reactor.cpp python/kvikio/tests/test_http_io.pypython3 -m py_compile python/kvikio/tests/test_http_io.pygit diff --checkNot run locally: the KvikIO Python/C++ test suite, because this checkout does not have an importable
kvikiopackage and CUDA tooling/GPU access are unavailable (nvccandnvidia-smiare not present).