sqlite: handle conflicting SQLite and JS errors #56787
Merged
Conversation
This file contains 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
nodejs-github-bot
added
c++
|
As a probably worthless thought, the alternative way of handling double-dipped errors would be to use
|
|
That's not a bad idea. I'd prefer to leave that for a followup though. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56787 +/- ##
==========================================
- Coverage 89.16% 89.16% -0.01%
==========================================
Files 665 665
Lines 192602 192618 +16
Branches 37050 37054 +4
==========================================
+ Hits 171732 171746 +14
- Misses 13678 13685 +7
+ Partials 7192 7187 -5
|
cjihrig
force-pushed
the
dualing-errors
branch
from
f113c39 to
edb86cf
Compare
github-actions
bot
removed
the
request-ci
cjihrig
force-pushed
the
dualing-errors
branch
from
edb86cf to
af10079
Compare
github-actions
bot
removed
the
request-ci
|
This could use a review. It addresses a nasty bug where data could be accidentally lost. |
|
It might be worth having a @nodejs/sqlite team too. |
jasnell
reviewed
Jan 29, 2025
jasnell
approved these changes
Jan 29, 2025
github-actions
bot
removed
the
request-ci
cjihrig
added
author ready
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/56787 ✔ Done loading data for nodejs/node/pull/56787 ----------------------------------- PR info ------------------------------------ Title sqlite: handle conflicting SQLite and JS errors (#56787) Author Colin Ihrig <[email protected]> (@cjihrig) Branch cjihrig:dualing-errors -> nodejs:main Labels c++, author ready, commit-queue-squash, sqlite Commits 2 - sqlite: handle conflicting SQLite and JS errors - nit Committers 1 - cjihrig <[email protected]> PR-URL: https://github.com/nodejs/node/pull/56787 Fixes: https://github.com/nodejs/node/issues/56772 Reviewed-By: James M Snell <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/56787 Fixes: https://github.com/nodejs/node/issues/56772 Reviewed-By: James M Snell <[email protected]> -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - nit ℹ This PR was created on Mon, 27 Jan 2025 14:29:43 GMT ✔ Approvals: 1 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/56787#pullrequestreview-2582507369 ✘ This PR needs to wait 66 more hours to land (or 0 hours if there is one more approval) ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-01-30T20:31:35Z: https://ci.nodejs.org/job/node-test-pull-request/64857/ - Querying data for job/node-test-pull-request/64857/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/13079812165 |
cjihrig
force-pushed
the
dualing-errors
branch
from
3c3dd2a to
48eca2a
Compare
|
The push was to resolve a conflict. |
This commit adds support for the situation where SQLite is trying to report an error while JavaScript already has an exception pending. Fixes: nodejs#56772
cjihrig
force-pushed
the
dualing-errors
branch
from
48eca2a to
f61def8
Compare
github-actions
bot
removed
the
request-ci
cjihrig
added
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/56787 ✔ Done loading data for nodejs/node/pull/56787 ----------------------------------- PR info ------------------------------------ Title sqlite: handle conflicting SQLite and JS errors (#56787) Author Colin Ihrig <[email protected]> (@cjihrig) Branch cjihrig:dualing-errors -> nodejs:main Labels c++, author ready, commit-queue-squash, sqlite Commits 1 - sqlite: handle conflicting SQLite and JS errors Committers 1 - cjihrig <[email protected]> PR-URL: https://github.com/nodejs/node/pull/56787 Fixes: https://github.com/nodejs/node/issues/56772 Reviewed-By: James M Snell <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/56787 Fixes: https://github.com/nodejs/node/issues/56772 Reviewed-By: James M Snell <[email protected]> -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - sqlite: handle conflicting SQLite and JS errors ℹ This PR was created on Mon, 27 Jan 2025 14:29:43 GMT ✔ Approvals: 1 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/56787#pullrequestreview-2582507369 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-02-02T02:57:57Z: https://ci.nodejs.org/job/node-test-pull-request/64905/ - Querying data for job/node-test-pull-request/64905/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/13115731534 |
|
@jasnell would you mind refreshing your LGTM. I had to rebase this PR and now it can't land. |
targos
approved these changes
Feb 4, 2025
cjihrig
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in c4fb331 |
targos
pushed a commit
that referenced
this pull request
Feb 5, 2025
This commit adds support for the situation where SQLite is trying to report an error while JavaScript already has an exception pending. Fixes: #56772 PR-URL: #56787 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
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 commit adds support for the situation where SQLite is trying to report an error while JavaScript already has an exception pending.
Fixes: #56772
Opening this PR since there hasn't been a reply on #56773 yet.