-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-44337: [CI][GLib] Fix a flaky StreamDecoder and Buffer test #44341
Conversation
It's related to GC. StreamDecoder accepts incomplete data. They are kept until enough data are provided. A caller must not release the incomplete data before they are processed. If they are released, StreamDecoder may touch unexpected data.
|
@github-actions crossbow submit verify-rc-source-ruby-* |
|
@@ -79,8 +79,15 @@ def test_consume_bytes | |||
end | |||
|
|||
def test_consume_buffer | |||
# We need to keep data that aren't processed yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make any sense to proactively change test_consume_bytes
too? Line 71 in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. We don't need to change test_consume_bytes
.
arrow::ipc::StreamDecoder::Consume(const uint8_t* data, int64_t size)
copied the given data internally when they aren't enough size.
(arrow::ipc::StreamDecoder::Consume(std::shared_ptr<Buffer> buffer)
refers them instead of copying.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't the Buffer
object keep the Ruby object alive? PyArrow wouldn't have this problem AFAIR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, we can do it by creating a sub Buffer
class: #44349
|
Hmm... I don't know why I couldn't run Crossbow jobs... @assignUser Do you have any idea? (Is |
Anyway, I'll merge this because the test is also covered CI jobs in apache/arrow. |
@kou yeah the crossbow token might have expired I'll check it out |
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 38c1286. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
It's related to GC.
StreamDecoder accepts incomplete data. They are kept until enough data are provided. A caller must not release the incomplete data before they are processed. If they are released, StreamDecoder may touch unexpected data.
What changes are included in this PR?
Refer unprocessed data until they are processed.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.