zlib: add sync versions for convenience methods #6987
Conversation
|
Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion. The following commiters were not found in the CLA:
You can fix all these things without opening another issue. Please see CONTRIBUTING.md for more information |
| Context::Scope context_scope(env->context()); | ||
|
|
||
| // Acceptable error states depend on the type of zlib stream. | ||
| switch (ctx->err_) { |
There was a problem hiding this comment.
Please try to de-dupe it, perhaps a template version of AfterWork could handle it.
|
Generally, looks good to me. But needs documentation fixes too. |
|
Done. |
| // normal statuses, not fatal | ||
| break; | ||
| case Z_NEED_DICT: | ||
| if (ctx->dictionary_ == NULL) { |
There was a problem hiding this comment.
I know you copied from below, but please remove braces { }
|
Yay, almost ready! :) Thank you very much. Please correct last nits and let me know |
|
Done again! |
| }; | ||
|
|
||
| function zlibBuffer(engine, buffer, callback) { | ||
| var buffers = []; | ||
| var nread = 0; | ||
|
|
||
| if (!util.isFunction(callback)) { | ||
| if (util.isString(buffer)) |
There was a problem hiding this comment.
Sorry, but one more thing. Please move it to separate function as well.
|
Question: Shouldn't we choose one and unify the API accordingly? And as |
|
@thomseddon On the other hand, |
|
@thomseddon I agree with @seishun |
|
@seishun Good point well made :) |
|
Okay I know this is crazy but I couldn't think of a better way to de-dupe this. |
|
What? |
|
I'm not sure I understand the question. I was referring to my last commit. I didn't know how the core devs would feel about the callback approach in de-duping the recursive write but the idea seemed interesting to me so I did it anyway. Also it's 2:30 AM here so I'll probably refrain from writing anything else for today. |
|
Ah, you was referring to your last commit |
| }); | ||
|
|
||
| engine._processChunk(buffer, function(binding, args, callback) { | ||
| callback.apply(null, binding.writeSync.apply(binding, args)); |
There was a problem hiding this comment.
Good approach! But let's try to avoid calling apply, where possible.
And additional advice, you could pass true as an argument for async write and false for a synchronous write and call writeSync/write in the _processChunk.
|
|
||
| engine._processChunk(buffer, function(binding, args, callback) { | ||
| callback.apply(null, binding.writeSync.apply(binding, args)); | ||
| }, function(out) { |
There was a problem hiding this comment.
As another idea, you could use passed flag to determine the desirable behavior instead of passing a callbacks.
There was a problem hiding this comment.
I mean that you should not pass callback at all.
|
I like where it is going! So far no more comments, will continue after you'll fix it! |
|
Done! I figured that a loop would be a better fit for the synchronous version than recursion. |
| @@ -480,7 +531,12 @@ Zlib.prototype._transform = function(chunk, encoding, cb) { | |||
| var out = self._buffer.slice(self._offset, self._offset + have); | |||
| self._offset += have; | |||
| // serve some output to the consumer. | |||
| self.push(out); | |||
| if (async) | |||
There was a problem hiding this comment.
Please add braces here, when else has braces - if should have them too.
|
Actually, I think I could fix all that stuff myself... please wait a minute |
|
LGTM |
|
Thank you very much for all your efforts, landed in 9b37b83 |
The user should have an option to run CPU-bound functions synchronously. Zlib is the only case where they don't.
This patch should be considered as a draft or a proof-of-concept. Any directions on how it could be improved will be greatly appreciated.