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: use common owner symbol to access JS wrapper #23189

Closed
wants to merge 1 commit into from
Closed

zlib: use common owner symbol to access JS wrapper #23189

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
13 changes: 11 additions & 2 deletions lib/zlib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ const {
Buffer,
kMaxLength
} = require('buffer');
const { owner_symbol } = require('internal/async_hooks').symbols;

const constants = process.binding('constants').zlib;
const {
Expand Down Expand Up @@ -143,7 +144,7 @@ function zlibBufferSync(engine, buffer) {
}

function zlibOnError(message, errno) {
var self = this.jsref;
var self = this[owner_symbol];
// there is no way to cleanly recover.
// continuing only obscures problems.
_close(self);
Expand Down Expand Up @@ -300,7 +301,8 @@ function Zlib(opts, mode) {
Transform.call(this, opts);
this.bytesWritten = 0;
this._handle = new binding.Zlib(mode);
this._handle.jsref = this; // Used by processCallback() and zlibOnError()
// Used by processCallback() and zlibOnError()
this._handle[owner_symbol] = this;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use a non-enumerable property here? Or would this not show up when inspecting the object?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would, but it would show up only when inspecting the handle object. In that case, I think pointing to the JS wrapper is a good idea.

When inspecting the JS wrapper, this shows up as [Circular], which seems okay to me.

Copy link
Member

Choose a reason for hiding this comment

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

I personally would rather hide it since the property itself is probably not useful for the regular user. I also prefer to hide such things so less users would try to abuse any internals. If someone wants to see these properties it's always possible by explicitly requesting 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.

In that case _handle is probably the thing to hide, not the owner on the handle.

One thing I know I’ve thought about a while ago is a custom inspection function for all native handles – not something that blends them out completely, but rather gives a useful overview over the current state in some way.

this._handle.onerror = zlibOnError;
this._hadError = false;
this._writeState = new Uint32Array(2);
Expand Down Expand Up @@ -713,6 +715,13 @@ function createProperty(ctor) {
};
}

// Legacy alias on the C++ wrapper object. This is not public API, so we may
// want to runtime-deprecate it at some point. There's no hurry, though.
Object.defineProperty(binding.Zlib.prototype, 'jsref', {
get() { return this[owner_symbol]; },
set(v) { return this[owner_symbol] = v; }
});

module.exports = {
Deflate,
Inflate,
Expand Down