fix: let a callback call igraph again - #2851
Open
krlmlr wants to merge 1 commit into
Open
Conversation
Every R callback igraph invokes from C could take the session down.
`R_igraph_finalizer()` calls `IGRAPH_FINALLY_FREE()`,
and every igraph function calls it from `on.exit()`
to clean up after an algorithm that was left half-way.
An igraph function reached from a callback is not that case:
the "finally" stack then holds the structures of the algorithm that is still
running, and freeing them leaves it working on destroyed memory.
The algorithm carries on and trips over an assertion, or takes R down with it.
Reaching igraph again from a callback needs no intent.
`length()` on a graph calls `vcount()`,
and lifecycle gets there through `rlang::trace_back()` while it assembles a
backtrace, so a deprecated function used as a callback was enough:
bfs(make_ring(5), root = 1, callback = function(...) { is.igraph(g); FALSE })
#> Error: At core/dqueue.pmt:120 : Assertion failed: q->stor_begin != NULL.
The finalizer now stands aside while a callback is running.
`Rx_igraph_eval_callback()` counts the callbacks in flight,
through `R_UnwindProtect()` so that the count comes back down even when the
callback raises an error and R unwinds past it.
Everything that hands R code to a running algorithm goes through it:
the search callbacks in `rcallback.c`, `Rx_igraph_safe_eval_in_env()` for
`bfs()`, `dfs()` and the attribute combination functions, and the ARPACK
multiplication.
The `bfs()` test that covered this is no longer skipped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WJx1uH2GvN5DcqdVzDLvwW
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR was prepared with Claude Code (Claude Opus).
Fixes #253. Found while working on #2845, where a deprecated function used as a callback turned out to take the session down.
The bug
Every R callback igraph invokes from C could crash.
bfs()is the shortest reproducer, but it is not specific to it:That is #253, open since 2018. It needs no intent to reproduce:
length()on a graph callsvcount(), and lifecycle gets there throughrlang::trace_back()while it assembles a backtrace, so a deprecated function used as a callback was enough:Every callback family is affected, and several take R down outright rather than erroring:
bfs(),dfs()— igraph assertioncliques(),max_cliques(),motifs(),simple_cycles()— igraph assertionisomorphisms(),subgraph_isomorphisms(),arpack()— R abortscluster_leading_eigen()— R abortssimplify(),contract()and the operators — igraph assertion, or R abortsWhy
R_igraph_finalizer()callsIGRAPH_FINALLY_FREE(), and every igraph function calls it fromon.exit()to clean up after an algorithm that was left half-way. An igraph function reached from a callback is not that case: the "finally" stack then holds the structures of the algorithm that is still running, and freeing them leaves it working on destroyed memory. The algorithm carries on and trips over an assertion, or writes into freed memory and takes R with it.The chain for the lifecycle case, from a traced
.igraph.progress():The fix
The finalizer stands aside while a callback is running.
Rx_igraph_eval_callback()counts the callbacks in flight and every path that hands R code to a running algorithm goes through it: the search callbacks inrcallback.c,Rx_igraph_safe_eval_in_env()forbfs(),dfs()and the attribute combination functions, and the ARPACK multiplication.The count is maintained with
R_UnwindProtect()rather than a plain decrement afterwards. A callback that raises an error unwinds past the eval, and a count left standing would silence the finalizer for the rest of the session — which is worse than the original bug, because cleanup after a genuine error then stops happening too. I know because that is what my first attempt did: it counted running algorithms aroundIGRAPH_R_CHECK, and the leak turned intofree(): invalid sizeseveral test files later.Nothing else changes. The algorithm cleans up after itself when it finishes, and the error path in
Rx_igraph_handle_safe_eval_result_in_env()still callsIGRAPH_FINALLY_FREE()before it aborts — that one runs after the callback has returned, so it is outside the guard.This is narrower than #2548, which would give the R glue a re-entrant
IGRAPH_FINALLYAPI with local stacks. That is still the thorough answer; this one stops the crashes without touching the API or the generated code.Testing
New
tests/testthat/test-nested-igraph-calls.Rcovers a nested igraph call from every callback family listed above, the deprecation case that led here, the reproducer from #253, and that an error raised in a callback still reaches the caller.The
bfs()test that covered this is no longer skipped:test_that("BFS callback does not blow up when another igraph function is raised within the callback", { - skip("nested igraph call handling not implemented yet")Full suite passes; the two failures seen locally are environmental (a blocked download in
test-foreign.R, and acallrsubprocess intest-other.Rthat needs an installed igraph).🤖 Generated with Claude Code
https://claude.ai/code/session_01WJx1uH2GvN5DcqdVzDLvwW
Generated by Claude Code