Skip to content

Commit

Permalink
zlib: reduce number of static internal methods
Browse files Browse the repository at this point in the history 
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 c072057.

PR-URL: #20674
Refs: c072057
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
@addaleax @MylesBorins
addaleax authored and MylesBorins committed May 22, 2018
1 parent 7d28f5b commit d5db576
Showing 1 changed file with 74 additions and 88 deletions.
162 changes: 74 additions & 88 deletions src/node_zlib.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}

Expand All @@ -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;
Expand All @@ -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:
Expand All @@ -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;
}

Expand All @@ -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<Function> cb = PersistentToLocal(env->isolate(),
ctx->write_js_callback_);
ctx->MakeCallback(cb, 0, nullptr);
Local<Function> 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 (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());
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<Value> 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<Value>& args) {
Expand Down Expand Up @@ -510,7 +496,7 @@ class ZCtx : public AsyncWrap, public ThreadPoolWork {
static void Reset(const FunctionCallbackInfo<Value> &args) {
ZCtx* ctx;
ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Holder());
Reset(ctx);
ctx->Reset();
SetDictionary(ctx);
}

Expand Down Expand Up @@ -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");
}
}

Expand All @@ -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");
}
}

Expand Down

0 comments on commit d5db576

Please sign in to comment.