fix(core): wait for NVD processing threads to stop before closing the database - #8819
VolodymyrLinuxovich wants to merge 1 commit into
Conversation
| private static void awaitTermination(ExecutorService service, String name) { | ||
| try { | ||
| if (!service.awaitTermination(SHUTDOWN_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { | ||
| LOGGER.warn("NVD {} threads did not stop within {} seconds; interrupting them", name, SHUTDOWN_TIMEOUT_SECONDS); | ||
| service.shutdownNow(); | ||
| if (!service.awaitTermination(SHUTDOWN_TIMEOUT_SECONDS, TimeUnit.SECONDS)) { | ||
| LOGGER.warn("NVD {} threads are still running", name); | ||
| } | ||
| } | ||
| } catch (InterruptedException ex) { | ||
| service.shutdownNow(); | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Good catch. It now keeps waiting until the pool terminates and restores the interrupt flag afterwards. I also added a test for the interrupted-caller case.
|
I didn’t look at this in detail. With a quick view looked like a crazy of # of lines changed for what it said it was trying to achieve - which didn’t pass the “sanity” test for me. My concern was that it was doing what LlMs often do when improperly guided, and making things progressively more complicated and unreadable instead of making things simpler and more robust through selective/surgical design changes. |
… database The update used to shutdownNow() the processing pool and return straight away, so the engine closed the database while processors were still writing to it. Interrupting them also breaks H2, which closes its file when a thread is interrupted during I/O. Instead, set a shared cancelled flag that processors check between CVEs, stop the pools without interrupting the processors, and wait for them to finish before returning. Fixes dependency-check#8622
Fair point, it had grown. I've cut it back to one commit. It's shared cancelled flag that the processors check between CVEs, and waiting for the pools to stop instead of shutdownNow() and returning. I didn't interrupt the processors because interrupting H2 mid-write closes the file, which is the MVStoreException from the issue. The other changes (closed pool check, UpdateException, extra logging) are gone, and I can open separate PRs for those if they're wanted. There's one test that fails on main and passes here. |
6f1cc48 to
c1e9000
Compare
|
Looks like you'e just copy and pasting from your LLM or having an agent respond. If so, I'm done here. I choose to interact only with actual thinking humans in public. |
I do ask an LLM for help sometimes. But I'm still learning how to work in a codebase this large, and I sometimes use an LLM to help me understand how the pieces fit together. I'm responsible for this PR, and I'm working to understand every change I submit. I took your feedback seriously and cut the patch back because I'm trying to make a useful contribution and learn from review. I understand if you'd rather not continue, sorry about it :( |
Description of Change
Fixes NVD update failures leaving processor threads running while the DB is closing.
getConnection()after DB close throwsDatabaseExceptioninstead of NPEprocessApi()failures are propagated asUpdateExceptionThis only covers NVD data source threads.
Related issues
Have test cases been added to cover the new functionality?
yes
Added tests for API failure, cancel/interrupt, closed DB handling, processor failure, and
getConnection()after close.Manual testing
Tested
--updateonlywith one NVD page followed by a 503.mvn -pl core verifypasses exceptGolangModAnalyzerTestbecausegois not installed.