-
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
ARROW-17252: [R] Intermittent valgrind failure #13746
Conversation
@github-actions crossbow submit test-r-linux-valgrind |
|
Revision: cb306e6 Submitted crossbow builds: ursacomputing/crossbow @ actions-cd6a907a1c
|
@github-actions crossbow submit test-r-linux-valgrind |
Revision: 0e7fdbd Submitted crossbow builds: ursacomputing/crossbow @ actions-6eeadbed1b
|
@github-actions crossbow submit test-r-linux-valgrind |
Revision: b72ac2c Submitted crossbow builds: ursacomputing/crossbow @ actions-46a81cabd4
|
@github-actions crossbow submit test-r-linux-valgrind |
Revision: a5a5011 Submitted crossbow builds: ursacomputing/crossbow @ actions-d4dfafaa47
|
@github-actions crossbow submit test-r-linux-valgrind |
Revision: db12bff Submitted crossbow builds: ursacomputing/crossbow @ actions-485782cd0a
|
@github-actions crossbow submit test-r-linux-valgrind |
Revision: 690a9a0 Submitted crossbow builds: ursacomputing/crossbow @ actions-4c128215b9
|
This PR fixes intermittent leaks that occur after one of the changes from ARROW-16444: when we drain the `RecordBatchReader` that is emitted from the plan too quickly, it seems, some parts of the plan can leak (I don't know why this happens). I tried removing various pieces of the `RunWithCapturedR()` changes (see #13746) but the only thing that removes the errors completely is draining the resulting `RecordBatchReader` from R (i.e., `reader$read_table()`) instead of in C++ (i.e., `reader->ToTable()`). Unfortunately, for user-defined functions to work in a plan we need a C++ level `reader->ToTable()`. I took the approach here of disabling the C++ level read by default, requiring a user to opt in to the version of `collect()` that works with a UDF. It's not ideal, but definitely safer (and more clearly marks the user-defined function behaviour as experimental). I was able to replicate the original leaks but they are few and far between...our tests just happen to create and destroy many, many exec plans and something about the CI environment seems to trigger these more reliably (although the errors don't always occur there, either). Most of the leaks are small but there were some instances where an entire `Table` leaked. Authored-by: Dewey Dunnington <[email protected]> Signed-off-by: Neal Richardson <[email protected]>
No description provided.