zlib: throw brotli initialization error from c++ #54698
Closed
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++
anonrig
force-pushed
the
yagiz/zlib-throw-err
branch
3 times, most recently
from
899e914
to
25d076f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54698 +/- ##
=======================================
Coverage 88.07% 88.07%
=======================================
Files 651 651
Lines 183538 183530 -8
Branches 35862 35862
=======================================
- Hits 161647 161645 -2
+ Misses 15136 15135 -1
+ Partials 6755 6750 -5
|
|
cc @nodejs/cpp-reviewers |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Flarna
approved these changes
Sep 3, 2024
This comment was marked as outdated.
This comment was marked as outdated.
jasnell
approved these changes
Sep 3, 2024
addaleax
previously requested changes
Sep 3, 2024
| @@ -816,14 +815,8 @@ function Brotli(opts, mode) { | |||
| new binding.BrotliDecoder(mode) : new binding.BrotliEncoder(mode); | |||
|
|
|||
| this._writeState = new Uint32Array(2); | |||
| // TODO(addaleax): Sometimes we generate better error codes in C++ land, | |||
| // e.g. ERR_BROTLI_PARAM_SET_FAILED -- it's hard to access them with | |||
| // the current bindings setup, though. | |||
There was a problem hiding this comment.
Please don't remove this TODO comment until it's actually addressed :)
There was a problem hiding this comment.
I think I misunderstood the TODO in here. Can you explain if you don't mind?
There was a problem hiding this comment.
Yeah, the TODO is about surfacing the actual error codes we get from the underlying library, instead of a generic "initialization failed" error code – ERR_BROTLI_PARAM_SET_FAILED being one example of that
There was a problem hiding this comment.
I reverted the change. Thank you for the review.
There was a problem hiding this comment.
Do you want to move the TODO comment along to the new throw sites?
There was a problem hiding this comment.
I moved it. There is 2 occurrences where we throw the error on C++. For the sake of reducability, I just kept the TODO on one of them.
anonrig
force-pushed
the
yagiz/zlib-throw-err
branch
from
25d076f
to
7dd40f8
Compare
Flarna
approved these changes
Sep 11, 2024
anonrig
added
the
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/54698 ✔ Done loading data for nodejs/node/pull/54698 ----------------------------------- PR info ------------------------------------ Title zlib: throw brotli initialization error from c++ (#54698) Author Yagiz Nizipli <[email protected]> (@anonrig) Branch anonrig:yagiz/zlib-throw-err -> nodejs:main Labels c++, zlib, lib / src, needs-ci Commits 1 - zlib: throw brotli initialization error from c++ Committers 1 - Yagiz Nizipli <[email protected]> PR-URL: https://github.com/nodejs/node/pull/54698 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/54698 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Sun, 01 Sep 2024 16:50:17 GMT ✘ Requested Changes: 1 ✘ - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/54698#pullrequestreview-2278408395 ✔ Approvals: 3 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/54698#pullrequestreview-2275983863 ✔ - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/54698#pullrequestreview-2297007257 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54698#pullrequestreview-2277663481 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-09-11T02:55:54Z: https://ci.nodejs.org/job/node-test-pull-request/62297/ - Querying data for job/node-test-pull-request/62297/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/10812657737 |
|
@anonrig feel free to dismiss my review if you think the TODO comment should remain where it is (it doesn't make sense to me in its current location where it's pretty unclear now what kind of error it's talking about) |
I'll move the comment location, just didn't had any time yet. |
anonrig
force-pushed
the
yagiz/zlib-throw-err
branch
from
7dd40f8
to
fef6ffc
Compare
RedYetiDev
removed
the
commit-queue-failed
jasnell
approved these changes
Sep 21, 2024
jasnell
pushed a commit
that referenced
this pull request
Sep 21, 2024
PR-URL: #54698 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Landed in 5b3f3c5 |
targos
pushed a commit
that referenced
this pull request
Oct 4, 2024
PR-URL: #54698 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[email protected]>
Merged
louwers
pushed a commit
to louwers/node
that referenced
this pull request
Nov 2, 2024
PR-URL: nodejs#54698 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[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.
Completes a 6 year old TODO by @addaleax introduced at 73753d4