Extend ObjectWrap instead of EventEmitter for node v0.6.0 compatibilty #9

Open
wants to merge 1 commit into
from

Projects

None yet

4 participants

@tungj

This makes the Gzip, Gunzip, Bzip and Bunzip classes extend ObjectWrap instead of EventEmitter since the latter has been removed from node v0.5 and the stable v0.6.0, as described in Woodya/node-gzbz2#8

I'm not certain I'm doing this correctly, but 'node test.js' does work in both node v0.4.11 and v0.6.0 (assuming the symlink to gzbz2 is updated to point to build/debug instead of build/Release which v0.6.0 uses)

@alex-kocharin

it works, thanks

@Woodya
Owner

@tungj can you explain the static Persistent s_ct; part of the change?

@ripcurld00d

@Woodya I have no idea why he set Persistent<Function> to be static. However, he is right that we have to use Persistent<Function> and pass it to target->Set.
According to Chrome V8 developers guide (I think):

A handle provides a reference to a JavaScript object's location in the heap. 
The V8 garbage collector reclaims memory used by objects that can no longer
again be accessed.[...]

There are several types of handles:
   * Local handles are held on a stack and are deleted when the appropriate destructor
     is called. These handles' lifetime is determined by a handle scope, which is often
     created at the beginning of a function call. [...]
   * Persistent handles provide a reference to a heap-allocated JavaScript Object,
     just like a local handle.[...]  Use a persistent handle when you need to keep
     a reference to an object for more than one function call, or when handle lifetimes
     do not correspond to C++ scopes.

https://developers.google.com/v8/embed

@Woodya
Owner

@ripcurld00d so i've read that section now, and i don't yet see that Persistent is correct v Local in this case.

... Use a persistent handle when you need to keep a reference to an object for more than one function call, or when handle lifetimes do not correspond to C++ scopes ...

this is not true in this case, at least afaict. The handle is only used inside the Initialize (static) method of each class, which in turn is called from the extern'd init().

I do see that various stackoverflow and web tutorials use the " .. s_ct = new .. (t)" pattern, but i don't see how any of them are more correct. It seems to me that they simply burden the system with a minor additional heap allocation. B/c unless a second function call will use the object, it's pointless.
for reference, the links i found in favor of this methodology:

however (and if i'm still wrong here, please point me in the right direction), i think they're simply re-using a pattern isn't appropriate, as they never use the s_ct outside their 1 function.
the s_ct pattern is not used in the nodejs addon docs for class prototype definitions

Also, per above comment about target->Set(), the commit actually leaves that reference as
target->Set(String::NewSymbol("Gzip"), t->GetFunction());

in short, i think it's an unnecessary involvement of the heap & additional unused symbols in the class.
I don't think it's needed.

@ripcurld00d

OK, I haven't done any deep investigation but from what I have seen so far, the Persistent in gzbz2 seems useless. Nevertheless, I will submit a PR with more testings (we don't test bz2/bunzip + memory leaks) just to cover more scenarios and make sure there will be no regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment