Skip to content
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

[R] Failed allocations call into R from a non-R thread #37576

Closed
paleolimbot opened this issue Sep 5, 2023 · 0 comments · Fixed by #37565
Closed

[R] Failed allocations call into R from a non-R thread #37576

paleolimbot opened this issue Sep 5, 2023 · 0 comments · Fixed by #37565

Comments

@paleolimbot
Copy link
Member

Describe the bug, including details regarding any error messages, version, and platform.

The gc_memory_pool() is the one we use almost everywhere in the R package. It uses a special allocation mechanism that calls into R to run the garbage collector after a failed allocation (in case there are any large objects that can be removed). In the case where an allocation happens on another thread (most of the time when running exec plans), the call into R may cause a crash: even though the memory pool was ensuring serialized access using a mutex, this is not sufficient for R and this sort of code normally crashes R or leads to confusing errors.

Component(s)

R

paleolimbot added a commit that referenced this issue Sep 18, 2023
… failed allocation (#37565)

### Rationale for this change

The `gc_memory_pool()` is the one we use almost everywhere in the R package. It uses a special allocation mechanism that calls into R to run the garbage collector after a failed allocation (in case there are any large objects that can be removed). In the case where an allocation happens on another thread (most of the time when running exec plans), the call into R may cause a crash: even though the memory pool was ensuring serialized access using a mutex, this is not sufficient for R (for reasons I don't understand).

### What changes are included in this PR?

Use `SafeCallIntoR()` to run the garbage collector instead. This ensures that the calling thread is used for any call into R (or errors if this is not possible).

### Are these changes tested?

Yes: there is an existing test that ensures this code path occurs at least once.

### Are there any user-facing changes?

No.
* Closes: #37576

Authored-by: Dewey Dunnington <dewey@voltrondata.com>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
@paleolimbot paleolimbot added this to the 14.0.0 milestone Sep 18, 2023
loicalleyne pushed a commit to loicalleyne/arrow that referenced this issue Nov 13, 2023
…fter a failed allocation (apache#37565)

### Rationale for this change

The `gc_memory_pool()` is the one we use almost everywhere in the R package. It uses a special allocation mechanism that calls into R to run the garbage collector after a failed allocation (in case there are any large objects that can be removed). In the case where an allocation happens on another thread (most of the time when running exec plans), the call into R may cause a crash: even though the memory pool was ensuring serialized access using a mutex, this is not sufficient for R (for reasons I don't understand).

### What changes are included in this PR?

Use `SafeCallIntoR()` to run the garbage collector instead. This ensures that the calling thread is used for any call into R (or errors if this is not possible).

### Are these changes tested?

Yes: there is an existing test that ensures this code path occurs at least once.

### Are there any user-facing changes?

No.
* Closes: apache#37576

Authored-by: Dewey Dunnington <dewey@voltrondata.com>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…fter a failed allocation (apache#37565)

### Rationale for this change

The `gc_memory_pool()` is the one we use almost everywhere in the R package. It uses a special allocation mechanism that calls into R to run the garbage collector after a failed allocation (in case there are any large objects that can be removed). In the case where an allocation happens on another thread (most of the time when running exec plans), the call into R may cause a crash: even though the memory pool was ensuring serialized access using a mutex, this is not sufficient for R (for reasons I don't understand).

### What changes are included in this PR?

Use `SafeCallIntoR()` to run the garbage collector instead. This ensures that the calling thread is used for any call into R (or errors if this is not possible).

### Are these changes tested?

Yes: there is an existing test that ensures this code path occurs at least once.

### Are there any user-facing changes?

No.
* Closes: apache#37576

Authored-by: Dewey Dunnington <dewey@voltrondata.com>
Signed-off-by: Dewey Dunnington <dewey@fishandwhistle.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants