posit-dev / posit-dev/positron
R: Errors deep in ALTREP methods can crash the kernel (duckplyr)
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 4.3k
- Forks
- 184
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 206
Description
See https://github.com/posit-dev/positron/issues/4158, quoted below:
"Not only this: errors in the materialization crash the kernel.
Downstream: https://github.com/tidyverse/duckplyr/issues/631
Simpler example: Run one of the two following lines in a Positron session:
x <- vctrs:::new_lazy_character(~ { stop("Called!"); letters })
x <- vctrs:::new_lazy_character(~ 1:3 )
"
This is mostly a problem with duckplyr, where things like INTEGER_ELT() can force an entire duckdb query to run, possibly resulting in an OOM error or something similar.
Outside of ALTREP, C functions like INTEGER_ELT() are supposed to be very safe to call. They don't error unless the caller has done something incorrectly. In ark, we have built a lot of infrastructure relying on this. This means that when these functions do error, we aren't prepared for it and the kernel can crash.
We've fixed some of this by instead forcing duckplyr related code to be called through R hooks that duckplyr gets to register methods for https://github.com/posit-dev/ark/pull/745. Those hooks are called in a context where errors are okay. That helps with not materializing unnecessarily, but doesn't really help with underlying problem.
It's impossible for us to tryCatch() around every function like INTEGER_ELT() that might have an ALTREP method (that would be way too slow, and the ergonomics would be awful as things like r_int_get() would have to return Result<i32>). So some other solution here would be needed.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reading the linked Positron issue 4158, duckplyr issue 631, and ark PR 745 to understand the existing hook-based mitigation. Reproduce the two ALTREP examples in a Positron session and trace how errors from INTEGER_ELT() or similar methods reach the kernel. Done means errors during ALTREP materialization no longer crash the kernel without adding tryCatch() around every such function.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- r
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100