Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zlib: throw brotli initialization error from c++ #54698

Closed
wants to merge 1 commit into from
Closed

zlib: throw brotli initialization error from c++ #54698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1886,4 +1886,3 @@ E('ERR_WORKER_UNSERIALIZABLE_ERROR',
'Serializing an uncaught exception failed', Error);
E('ERR_WORKER_UNSUPPORTED_OPERATION',
'%s is not supported in workers', TypeError);
E('ERR_ZLIB_INITIALIZATION_FAILED', 'Initialization failed', Error);
10 changes: 1 addition & 9 deletions lib/zlib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ const {
ERR_BUFFER_TOO_LARGE,
ERR_INVALID_ARG_TYPE,
ERR_OUT_OF_RANGE,
ERR_ZLIB_INITIALIZATION_FAILED,
},
genericNodeError,
} = require('internal/errors');
Expand Down Expand Up @@ -821,14 +820,7 @@ 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't remove this TODO comment until it's actually addressed :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I misunderstood the TODO in here. Can you explain if you don't mind?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted the change. Thank you for the review.

Copy link
Member

@addaleax addaleax Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to move the TODO comment along to the new throw sites?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!handle.init(brotliInitParamsArray,
this._writeState,
processCallback)) {
throw new ERR_ZLIB_INITIALIZATION_FAILED();
}
handle.init(brotliInitParamsArray, this._writeState, processCallback);

ReflectApply(ZlibBase, this, [opts, mode, handle, brotliDefaultOpts]);
}
Expand Down
1 change: 1 addition & 0 deletions src/node_errors.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ void OOMErrorHandler(const char* location, const v8::OOMDetails& details);
V(ERR_VM_MODULE_CACHED_DATA_REJECTED, Error) \
V(ERR_VM_MODULE_LINK_FAILURE, Error) \
V(ERR_WASI_NOT_STARTED, Error) \
V(ERR_ZLIB_INITIALIZATION_FAILED, Error) \
V(ERR_WORKER_INIT_FAILED, Error) \
V(ERR_PROTO_ACCESS, Error)

Expand Down
14 changes: 10 additions & 4 deletions src/node_zlib.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

#include "async_wrap-inl.h"
#include "env-inl.h"
#include "node_errors.h"
#include "node_external_reference.h"
#include "threadpoolwork-inl.h"
#include "util-inl.h"
Expand Down Expand Up @@ -271,6 +272,8 @@ class CompressionStream : public AsyncWrap, public ThreadPoolWork {
CHECK_EQ(unreported_allocations_, 0);
}

Environment* env() const { return this->ThreadPoolWork::env(); }

void Close() {
if (write_in_progress_) {
pending_close_ = true;
Expand Down Expand Up @@ -694,7 +697,11 @@ class BrotliCompressionStream final :
static_cast<CompressionStream<CompressionContext>*>(wrap));
if (err.IsError()) {
wrap->EmitError(err);
args.GetReturnValue().Set(false);
// 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.
THROW_ERR_ZLIB_INITIALIZATION_FAILED(wrap->env(),
"Initialization failed");
return;
}

Expand All @@ -708,12 +715,11 @@ class BrotliCompressionStream final :
err = wrap->context()->SetParams(i, data[i]);
if (err.IsError()) {
wrap->EmitError(err);
args.GetReturnValue().Set(false);
THROW_ERR_ZLIB_INITIALIZATION_FAILED(wrap->env(),
"Initialization failed");
return;
}
}

args.GetReturnValue().Set(true);
}

static void Params(const FunctionCallbackInfo<Value>& args) {
Expand Down
Loading