From ace4f0e1b5c0ce372cad8726db066572bc507598 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 11 May 2018 15:25:20 +0200 Subject: [PATCH 1/2] zlib: reduce number of static internal methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There really isn’t any good reason for these to be static methods, it just adds one layer of indirection (when reading the code, not in a way that affects behaviour). Addresses a `TODO` comment introduced in c072057049. Refs: https://github.com/nodejs/node/commit/c0720570498895d06dcec4e8f01e8922a81ac78b --- src/node_zlib.cc | 162 ++++++++++++++++++++++------------------------- 1 file changed, 74 insertions(+), 88 deletions(-) diff --git a/src/node_zlib.cc b/src/node_zlib.cc index ac5083862de4e3..8bd64a322dc89b 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -202,7 +202,7 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork { // sync version env->PrintSyncTrace(); ctx->DoThreadPoolWork(); - if (CheckError(ctx)) { + if (ctx->CheckError()) { ctx->write_result_[0] = ctx->strm_.avail_out; ctx->write_result_[1] = ctx->strm_.avail_in; ctx->write_in_progress_ = false; @@ -215,53 +215,43 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork { ctx->ScheduleWork(); } - // TODO(addaleax): Make these methods non-static. It's a significant bunch - // of churn that's better left for a separate PR. - void DoThreadPoolWork() override { - Process(this); - } - - void AfterThreadPoolWork(int status) override { - After(this, status); - } - // thread pool! // This function may be called multiple times on the uv_work pool // for a single write() call, until all of the input bytes have // been consumed. - static void Process(ZCtx* ctx) { + void DoThreadPoolWork() { const Bytef* next_expected_header_byte = nullptr; // If the avail_out is left at 0, then it means that it ran out // of room. If there was avail_out left over, then it means // that all of the input was consumed. - switch (ctx->mode_) { + switch (mode_) { case DEFLATE: case GZIP: case DEFLATERAW: - ctx->err_ = deflate(&ctx->strm_, ctx->flush_); + err_ = deflate(&strm_, flush_); break; case UNZIP: - if (ctx->strm_.avail_in > 0) { - next_expected_header_byte = ctx->strm_.next_in; + if (strm_.avail_in > 0) { + next_expected_header_byte = strm_.next_in; } - switch (ctx->gzip_id_bytes_read_) { + switch (gzip_id_bytes_read_) { case 0: if (next_expected_header_byte == nullptr) { break; } if (*next_expected_header_byte == GZIP_HEADER_ID1) { - ctx->gzip_id_bytes_read_ = 1; + gzip_id_bytes_read_ = 1; next_expected_header_byte++; - if (ctx->strm_.avail_in == 1) { + if (strm_.avail_in == 1) { // The only available byte was already read. break; } } else { - ctx->mode_ = INFLATE; + mode_ = INFLATE; break; } @@ -272,12 +262,12 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork { } if (*next_expected_header_byte == GZIP_HEADER_ID2) { - ctx->gzip_id_bytes_read_ = 2; - ctx->mode_ = GUNZIP; + gzip_id_bytes_read_ = 2; + mode_ = GUNZIP; } else { // There is no actual difference between INFLATE and INFLATERAW // (after initialization). - ctx->mode_ = INFLATE; + mode_ = INFLATE; } break; @@ -289,39 +279,37 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork { case INFLATE: case GUNZIP: case INFLATERAW: - ctx->err_ = inflate(&ctx->strm_, ctx->flush_); + err_ = inflate(&strm_, flush_); // If data was encoded with dictionary (INFLATERAW will have it set in // SetDictionary, don't repeat that here) - if (ctx->mode_ != INFLATERAW && - ctx->err_ == Z_NEED_DICT && - ctx->dictionary_ != nullptr) { + if (mode_ != INFLATERAW && + err_ == Z_NEED_DICT && + dictionary_ != nullptr) { // Load it - ctx->err_ = inflateSetDictionary(&ctx->strm_, - ctx->dictionary_, - ctx->dictionary_len_); - if (ctx->err_ == Z_OK) { + err_ = inflateSetDictionary(&strm_, dictionary_, dictionary_len_); + if (err_ == Z_OK) { // And try to decode again - ctx->err_ = inflate(&ctx->strm_, ctx->flush_); - } else if (ctx->err_ == Z_DATA_ERROR) { + err_ = inflate(&strm_, flush_); + } else if (err_ == Z_DATA_ERROR) { // Both inflateSetDictionary() and inflate() return Z_DATA_ERROR. // Make it possible for After() to tell a bad dictionary from bad // input. - ctx->err_ = Z_NEED_DICT; + err_ = Z_NEED_DICT; } } - while (ctx->strm_.avail_in > 0 && - ctx->mode_ == GUNZIP && - ctx->err_ == Z_STREAM_END && - ctx->strm_.next_in[0] != 0x00) { + while (strm_.avail_in > 0 && + mode_ == GUNZIP && + err_ == Z_STREAM_END && + strm_.next_in[0] != 0x00) { // Bytes remain in input buffer. Perhaps this is another compressed // member in the same archive, or just trailing garbage. // Trailing zero bytes are okay, though, since they are frequently // used for padding. - Reset(ctx); - ctx->err_ = inflate(&ctx->strm_, ctx->flush_); + Reset(); + err_ = inflate(&strm_, flush_); } break; default: @@ -336,27 +324,27 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork { } - static bool CheckError(ZCtx* ctx) { + bool CheckError() { // Acceptable error states depend on the type of zlib stream. - switch (ctx->err_) { + switch (err_) { case Z_OK: case Z_BUF_ERROR: - if (ctx->strm_.avail_out != 0 && ctx->flush_ == Z_FINISH) { - ZCtx::Error(ctx, "unexpected end of file"); + if (strm_.avail_out != 0 && flush_ == Z_FINISH) { + Error("unexpected end of file"); return false; } case Z_STREAM_END: // normal statuses, not fatal break; case Z_NEED_DICT: - if (ctx->dictionary_ == nullptr) - ZCtx::Error(ctx, "Missing dictionary"); + if (dictionary_ == nullptr) + Error("Missing dictionary"); else - ZCtx::Error(ctx, "Bad dictionary"); + Error("Bad dictionary"); return false; default: // something else. - ZCtx::Error(ctx, "Zlib error"); + Error("Zlib error"); return false; } @@ -365,59 +353,57 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork { // v8 land! - static void After(ZCtx* ctx, int status) { - Environment* env = ctx->env(); - ctx->write_in_progress_ = false; + void AfterThreadPoolWork(int status) { + write_in_progress_ = false; if (status == UV_ECANCELED) { - ctx->Close(); + Close(); return; } CHECK_EQ(status, 0); - HandleScope handle_scope(env->isolate()); - Context::Scope context_scope(env->context()); + HandleScope handle_scope(env()->isolate()); + Context::Scope context_scope(env()->context()); - if (!CheckError(ctx)) + if (!CheckError()) return; - ctx->write_result_[0] = ctx->strm_.avail_out; - ctx->write_result_[1] = ctx->strm_.avail_in; + write_result_[0] = strm_.avail_out; + write_result_[1] = strm_.avail_in; // call the write() cb - Local cb = PersistentToLocal(env->isolate(), - ctx->write_js_callback_); - ctx->MakeCallback(cb, 0, nullptr); + Local cb = PersistentToLocal(env()->isolate(), + write_js_callback_); + MakeCallback(cb, 0, nullptr); - ctx->Unref(); - if (ctx->pending_close_) - ctx->Close(); + Unref(); + if (pending_close_) + Close(); } - static void Error(ZCtx* ctx, const char* message) { - Environment* env = ctx->env(); - + // TODO(addaleax): Switch to modern error system. + void Error(const char* message) { // If you hit this assertion, you forgot to enter the v8::Context first. - CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); + CHECK_EQ(env()->context(), env()->isolate()->GetCurrentContext()); - if (ctx->strm_.msg != nullptr) { - message = ctx->strm_.msg; + if (strm_.msg != nullptr) { + message = strm_.msg; } - HandleScope scope(env->isolate()); + HandleScope scope(env()->isolate()); Local args[2] = { - OneByteString(env->isolate(), message), - Number::New(env->isolate(), ctx->err_) + OneByteString(env()->isolate(), message), + Number::New(env()->isolate(), err_) }; - ctx->MakeCallback(env->onerror_string(), arraysize(args), args); + MakeCallback(env()->onerror_string(), arraysize(args), args); // no hope of rescue. - if (ctx->write_in_progress_) - ctx->Unref(); - ctx->write_in_progress_ = false; - if (ctx->pending_close_) - ctx->Close(); + if (write_in_progress_) + Unref(); + write_in_progress_ = false; + if (pending_close_) + Close(); } static void New(const FunctionCallbackInfo& args) { @@ -510,7 +496,7 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork { static void Reset(const FunctionCallbackInfo &args) { ZCtx* ctx; ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Holder()); - Reset(ctx); + ctx->Reset(); SetDictionary(ctx); } @@ -613,7 +599,7 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork { } if (ctx->err_ != Z_OK) { - ZCtx::Error(ctx, "Failed to set dictionary"); + ctx->Error("Failed to set dictionary"); } } @@ -630,30 +616,30 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork { } if (ctx->err_ != Z_OK && ctx->err_ != Z_BUF_ERROR) { - ZCtx::Error(ctx, "Failed to set parameters"); + ctx->Error("Failed to set parameters"); } } - static void Reset(ZCtx* ctx) { - ctx->err_ = Z_OK; + void Reset() { + err_ = Z_OK; - switch (ctx->mode_) { + switch (mode_) { case DEFLATE: case DEFLATERAW: case GZIP: - ctx->err_ = deflateReset(&ctx->strm_); + err_ = deflateReset(&strm_); break; case INFLATE: case INFLATERAW: case GUNZIP: - ctx->err_ = inflateReset(&ctx->strm_); + err_ = inflateReset(&strm_); break; default: break; } - if (ctx->err_ != Z_OK) { - ZCtx::Error(ctx, "Failed to reset stream"); + if (err_ != Z_OK) { + Error("Failed to reset stream"); } } From 2e926888f06246125c7d30105903fd398743e28a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 13 May 2018 17:22:02 +0200 Subject: [PATCH 2/2] [squash] refer to node_errors.h in TODO comment --- src/node_zlib.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 8bd64a322dc89b..632dad1f8110be 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -382,7 +382,7 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork { Close(); } - // TODO(addaleax): Switch to modern error system. + // TODO(addaleax): Switch to modern error system (node_errors.h). void Error(const char* message) { // If you hit this assertion, you forgot to enter the v8::Context first. CHECK_EQ(env()->context(), env()->isolate()->GetCurrentContext());