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
+11 −2
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
zlib: use common owner symbol to access JS wrapper 
Use the same symbol that other `AsyncWrap` instances also use
for accessing the JS wrapper.
@addaleax
addaleax committed Oct 1, 2018

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9
Expired
Verified
Learn about vigilant mode
commit a30a39984ac1b241ae3c105b9a45f5dd5ed068d5
13 changes: 11 additions & 2 deletions lib/zlib.js
View file
Original file line number Diff line number Diff line change
@@ -42,6 +42,7 @@ const {
Buffer,
kMaxLength
} = require('buffer');
const { owner_symbol } = require('internal/async_hooks').symbols;

const constants = process.binding('constants').zlib;
const {
@@ -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);
@@ -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);
@@ -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,