feat: improve pg write queue #1878
Conversation
Signed-off-by: bestmike007 <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1878 +/- ##
========================================
Coverage 71.87% 71.87%
========================================
Files 92 92
Lines 11913 11920 +7
Branches 2624 2625 +1
========================================
+ Hits 8562 8568 +6
- Misses 3197 3198 +1
Partials 154 154 ☔ View full report in Codecov by Sentry. |
| done(): Promise<void> { | ||
| return this.queue.onIdle(); | ||
| async done(): Promise<void> { | ||
| // https://medium.com/@alkor_shikyaro/transactions-and-promises-in-node-js-ca5a3aeb6b74 |
There was a problem hiding this comment.
I don't believe the problem described in this post applies to the tasks handled by this queue for a couple of reasons:
PQueue'sonIdlealready behaves likeallSettledReturns a promise that settles when the queue becomes empty, and all promises have completed
- The API only enqueues work to this queue inside of a pre-existing SQL transaction (all the block processing code is contained in a single
sqlWriteTransaction). If there's ever an exception thrown inside, the transaction aborts and rolls back all the pending writes. We've done extensive tests on this and it's worked correctly for a long time.
There was a problem hiding this comment.
- That's what I thought, but it was wrong; here's a demonstration: https://replit.com/@bestmike007/TroubledEarnestSandboxes#index.ts
onIdledoes not throw an exception, it simply waits all promises to complete.
What I'm seeing is, there was an unhandledRejection and then the process restarted.
|
@bestmike007 I'm curious to now if this fix was submitted because of a problem you're experiencing? We'd be happy to test some edge case if you've found one we haven't covered yet |
|
Yes I was trying to resolve a problem, our nodes run into unrecoverable state, not sure if it's related to this PR. Here's one of the situations I saw, which should not be happening:
Another one, I couldn't found a screenshot, was reporting something like |
Signed-off-by: bestmike007 <[email protected]>
|
Hmm I don't believe that error is related to this change. It seems strange, though, because the |
| } | ||
| enqueue(task: Parameters<PQueue['add']>[0]): void { | ||
| void this.queue.add(task); | ||
| const p = this.queue.add(task); | ||
| p.catch(e => logger.error(e, 'PgWriteQueue task failed')); |
There was a problem hiding this comment.
Is this catch needed, or would it be handled later in the throw firstRejected.reason code?
If the error is logged here, it would have no meaningful stack trace compared to the below code which preserves the async call stack.
There was a problem hiding this comment.
If there's no catch, the process will be terminated by an unhandledRejection.
The error is logged here because throw firstRejected.reason only throws the first one, we don't have any log for the other errors.
There was a problem hiding this comment.
Ah okay, would it make sense to use the p-queue error event instead? https://www.npmjs.com/package/p-queue#error
There was a problem hiding this comment.
This is what we saw using a node built from this PR.
There was a problem hiding this comment.
I see, we can manually capture the errors ourself with .catch to prevent a terminated by an unhandledRejection. But could we instead use the build-in p-queue errror handler, e.g.
queue.on('error', error => {
console.error(error);
});
?
There was a problem hiding this comment.
This is not available before v7.0.0: https://github.com/sindresorhus/p-queue/tree/v6.6.2?tab=readme-ov-file#events
It it not related, it's another issue I saw when the node keeps crashing because of unhandledRejection when it fails to process a new_block event. Our nodes are set up using the script in this repo: https://github.com/alexgo-io/stacks-node-mainnet, and nodes restoring from both hiro archive and the cold backup saw this issue. |
Description
This PR is to fix the potential issue described in https://medium.com/@alkor_shikyaro/transactions-and-promises-in-node-js-ca5a3aeb6b74
Type of Change
Does this introduce a breaking change?
No.
Are documentation updates required?
No.
Testing information
Current tests covered this.
Checklist
npm run testpasses