Fix NPE in StandardJanusGraphTx.getVertices() on concurrent graph close (#4907) - #4914
Conversation
…se (JanusGraph#4907) getVertices() calls vertexCache.contains(id) followed by vertexCache.get(id, ...). If the transaction is released concurrently (e.g. graph.close() on another thread) between those two calls, the cache is swapped to an EmptyVertexCache whose get() returns null after getVertices() has already passed verifyOpen(). The null vertex was added to the result list and then dereferenced by result.removeIf(JanusGraphElement::isRemoved), throwing a NullPointerException. Guard against null entries in the removeIf predicate, aligning getVertices() with the null handling that getVertex() already has. Add a regression test that swaps in a vertex cache reproducing the contains()/get() race and asserts getVertices() no longer throws. Signed-off-by: Balmukund Trivedi <btrivedipublic@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Balmukund Trivedi <btrivedipublic@gmail.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes a concurrency-related NullPointerException in StandardJanusGraphTx.getVertices(Object...) when a concurrent transaction release swaps the vertexCache between contains() and get(), causing null entries to be added and later dereferenced during filtering.
Changes:
- Update
getVertices()to filter outnullentries in addition to removed vertices. - Add a regression test that simulates the
contains()/get()race (contains true, get null) and asserts null-safe behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| janusgraph-core/src/main/java/org/janusgraph/graphdb/transaction/StandardJanusGraphTx.java | Makes getVertices() null-safe by filtering out null results before checking isRemoved(). |
| janusgraph-test/src/test/java/org/janusgraph/graphdb/transaction/StandardJanusGraphTxGetVerticesTest.java | Adds a regression test that reproduces the cache race window and verifies getVertices() does not throw and excludes nulls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Fixes #4907.
StandardJanusGraphTx.getVertices(Object...)builds its result by callingvertexCache.contains(id)and thenvertexCache.get(id, ...). If the transaction is released concurrently — e.g.graph.close()on another thread — between those two calls,releaseTransaction()swapsvertexCacheto anEmptyVertexCachewhoseget()returnsnull, aftergetVertices()has already passedverifyOpen(). Thatnullwas added to the result list and then dereferenced by:throwing a
NullPointerExceptioninArrayList.removeIf.The singular
getVertex()method already guards against this (return (null == v || v.isRemoved()) ? null : v;). This change alignsgetVertices()with that behavior:Test
Adds
StandardJanusGraphTxGetVerticesTest, a regression test that swaps in a vertex cache reproducing thecontains()/get()race window (contains()returnstrue,get()returnsnull) and assertsgetVertices()no longer throws and filters out the null entry. The test fails with the exact NPE (ArrayList.removeIf→StandardJanusGraphTx.getVertices) before the fix and passes after it.🤖 Generated with Claude Code