fix(consumers): prevent await deadlock on ContextCallable failure

[bitflicker64]

Purpose of the PR

• close [Bug] Potential silent thread failure in Consumers.java due to unchecked Future #2939
• Fix possible await() hang when ContextCallable fails before entering runAndDone(). In that case latch.countDown() was never called, so await() could block forever and the failure could remain hidden.

Main Changes

Submit safeRun() instead of directly submitting ContextCallable

Add safeRun() wrapper to always call latch.countDown() in finally

Remove latch.countDown() from runAndDone() to avoid double-counting

Verifying these changes

• Trivial rework / code cleanup without any test coverage. (No Need)
• Already covered by existing tests, such as (please modify tests here).
• Need tests and can be verified as follows:
  • mvn -pl hugegraph-server/hugegraph-core -am test PASS; full build fails in hugegraph-clustertest-minicluster due to toolchain/JDK issue (unrelated).

Does this PR potentially affect the following parts?

• Dependencies (add/update license info & regenerate_known_dependencies.sh)
• Modify configurations
• The public API
• Other affects (typed here)
• Nope

Documentation Status

• Doc - TODO
• Doc - Done
• Doc - No Need

[imbajin]

Hi, thanks for your PR, maybe we need at least 2 tests to validate the modification

[Copilot]

Pull request overview

This PR fixes a potential deadlock issue in the Consumers class where await() could hang indefinitely if a ContextCallable fails before entering the runAndDone() method. The fix ensures that the countdown latch is always decremented, even when failures occur during worker thread initialization.

Changes:

• Introduced safeRun() wrapper method to guarantee latch.countDown() is called in all failure scenarios
• Modified worker submission to use safeRun() instead of directly submitting ContextCallable
• Moved latch countdown from runAndDone() to safeRun() to prevent double-counting while ensuring it always executes

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bitflicker64]

Pushed an update to this PR on branch fix/consumers-latch-deadlock.

Main changes

1. hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/util/Consumers.java

   • Start workers using safeRun() so latch.countDown() is guaranteed in a finally block even if ContextCallable fails before entering runAndDone().
   • Removed latch.countDown() from runAndDone() to avoid double countdown and ensure exactly one countdown per worker.
   • Minor style cleanup (removed an extra blank line). Kept LOG.error(...) for worker-start failures since these are real startup/thread-context errors.

2. Added 2 unit tests

   • File: hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/util/ConsumersTest.java
   • Tests:
     • testStartProvideAwaitNormal: verifies items are consumed and await() returns normally
     • testAwaitThrowsWhenConsumerThrows: verifies await() surfaces consumer exceptions (no hang)

Verification

• mvn -pl hugegraph-server/hugegraph-core -am test ✅ PASS locally

Thanks!

[bitflicker64]

Side note: there is another Consumers implementation at
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/job/algorithm/Consumers.java
which also submits ContextCallable directly. It may have the same latch/await hang corner case. I didn’t include it here to keep this PR focused, but I can open a separate PR to apply the same fix there if you want.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[imbajin]

🧹 Minor: The test doesn't verify the specific deadlock scenario mentioned in the PR description - where ContextCallable fails before entering runAndDone().

Consider adding a test case that explicitly simulates the failure scenario, for example:

@Test(timeout = 1000)
public void testAwaitDoesNotHangWhenContextCallableFails() throws Throwable {
    // Test that simulates ContextCallable constructor/call() failure
    // before runAndDone() is entered
}

[imbajin]

Side note: there is another Consumers implementation at hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/job/algorithm/Consumers.java which also submits ContextCallable directly. It may have the same latch/await hang corner case. I didn’t include it here to keep this PR focused, but I can open a separate PR to apply the same fix there if you want.

No need to update the another one (but we could merge them into one class/method reference)

[bitflicker64]

• The PR adds a safeRun() wrapper around the worker so latch.countDown() is guaranteed in a finally block. This is mainly to prevent await() from hanging if a worker fails early.

• Context propagation still needs to be preserved properly, so ContextCallable should be created/wrapped at submission time in start() , not inside the worker thread.

• avoid catch(Throwable) in runAndDone() since that would also catch Error (OOM/StackOverflow etc.). Keeping it to catch(Exception) matches the original behavior.

• Also making sure exceptionHandle() isn’t called redundantly for the same failure path, and tweaking the log message to something accurate (failure can be inside ContextCallable wrapper too, not strictly “before runAndDone”).

I’ll work on the test for where ContextCallable fails before entering runAndDone().

[codecov]

Codecov Report

❌ Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 1.57%. Comparing base (37be6cd) to head (ec746c9).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
...main/java/org/apache/hugegraph/util/Consumers.java	0.00%	18 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (37be6cd) and HEAD (ec746c9). Click for more details.

HEAD has 2 uploads less than BASE

Flag	BASE (37be6cd)	HEAD (ec746c9)
	4	2

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #2941       +/-   ##
============================================
- Coverage     39.38%   1.57%   -37.82%     
+ Complexity      456      43      -413     
============================================
  Files           812     779       -33     
  Lines         68660   65018     -3642     
  Branches       8968    8332      -636     
============================================
- Hits          27044    1026    -26018     
- Misses        38824   63906    +25082     
+ Partials       2792      86     -2706     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bitflicker64]

Thanks for the review. I’ve updated the implementation and All related unit tests pass locally.

[bitflicker64]

Side note: there is another Consumers implementation at hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/job/algorithm/Consumers.java which also submits ContextCallable directly. It may have the same latch/await hang corner case. I didn’t include it here to keep this PR focused, but I can open a separate PR to apply the same fix there if you want.

No need to update the another one (but we could merge them into one class/method reference)

When you said “no need to update the another one (but we could merge them into one class/method reference)”, did you mean that this PR should stay limited to the current Consumers, and that any fix for the other implementation should be handled separately possibly as a later refactor to share the common logic?

[imbajin]

Side note: there is another Consumers implementation at hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/job/algorithm/Consumers.java which also submits ContextCallable directly. It may have the same latch/await hang corner case. I didn’t include it here to keep this PR focused, but I can open a separate PR to apply the same fix there if you want.

No need to update the another one (but we could merge them into one class/method reference)

When you said “no need to update the another one (but we could merge them into one class/method reference)”, did you mean that this PR should stay limited to the current Consumers, and that any fix for the other implementation should be handled separately possibly as a later refactor to share the common logic?

Exactly! You got it.

We want to avoid code duplication across the project—ideally, these utilities should only exist in one place. If they need to be shared(across multi-modules), we can move them to hugegraph-commons in a later refactor.