Broken with io.js 3.x and node.js 4.x

[jdesboeufs]

Hi,

This library doesn't work with io.js 3.x so it will also break with upcoming node.js 4.0.
I tried to bump to Nan 2.0.x but some parts of utils must be rewritten, and it's over my skills.

I pushed the PR #113 to update Travis CI config.

[springmeyer]

Yes, Nan 2.x is - at least from experience with other modules - a pretty big lift. For reference my port of node-mapnik is at mapnik/node-mapnik#509

[jdesboeufs]

But very easy sometimes ;)
mapbox/node-srs#57

For node-gdal I think main difficulties are with utils/typed_array.cpp and utils/obj_cache.cpp.

[brandonreavis]

I can get on this soon, and yeah, the typed array and object cache stuff are a bit funky. Any rough timeframe on when you all want this done by?

[jdesboeufs]

We are following io.js versions for our Inspire platform but we can stay at 2.5.0 for now :)

[ChALkeR]

Adding to the list: nodejs/node#2798.

[jdesboeufs]

gdal is now the last package which stick us with iojs-v2 ;)
How can we help you?

[brandonreavis]

The reminder helps... I had forgotten about this. Thanks! I'll get on it this evening and I don't think it will take too long.

[brandonreavis]

I'm starting to make some progress on this, but the ObjectCache will need some considerable work because the undocumented _NanWeakCallbackInfo struct isn't around anymore and it relied on that to work. (nodejs/nan#283)

I'm really considering doing the caching purely in javascript + node-weak. The C++ version has been very brittle and it's going to be harder and harder to maintain. Also, currently the ObjectCache is only used for Datasets, SpatialReferences, Layers, RasterBands, and Drivers. These aren't created and destroyed nearly as much as things like features and geometry, so it shouldn't be a performance hit.

[brandonreavis]

Everything but the ObjectCache seems to be working now. That part however is being a pain...

I'm getting segfaults here whenever isNearDeath on the persistent handle is true. ObjectCache::getWrapped() is called from the Dataset destructor here.The goal of this is to call the dispose method on each of the child bands whenever a dataset is closed.

Stack Trace:

  ․ gdal.Dataset instance "bands" property forEach() should call callback for each RasterBand: 1ms
PID 30609 received SIGSEGV for address: 0xffffffffffffffff
0   segfault-handler.node               0x0000000100ffa91a _ZL16segfault_handleriP9__siginfoPv + 282
1   libsystem_platform.dylib            0x00007fff862c5f1a _sigtramp + 26
2   ???                                 0x0000000000000000 0x0 + 0
3   gdal.node                           0x000000010380f907 _ZN9node_gdal7Dataset7disposeEv + 735
4   gdal.node                           0x000000010380f5f7 _ZN9node_gdal7DatasetD2Ev + 25
5   gdal.node                           0x000000010380f9bc _ZN9node_gdal7DatasetD0Ev + 14
6   iojs                                0x00000001002c8ca1 _ZN2v88internal13GlobalHandles4Node31PostGarbageCollectionProcessingEPNS0_7IsolateE + 225
7   iojs                                0x00000001002c7887 _ZN2v88internal13GlobalHandles31PostGarbageCollectionProcessingENS0_16GarbageCollectorE + 103
8   iojs                                0x00000001002e75f5 _ZN2v88internal4Heap24PerformGarbageCollectionENS0_16GarbageCollectorENS_15GCCallbackFlagsE + 1205
9   iojs                                0x00000001002e6fb5 _ZN2v88internal4Heap14CollectGarbageENS0_16GarbageCollectorEPKcS4_NS_15GCCallbackFlagsE + 421
10  iojs                                0x00000001002e6849 _ZN2v88internal4Heap17CollectAllGarbageEiPKcNS_15GCCallbackFlagsE + 137
11  iojs                                0x0000000100165b96 _ZN2v87Isolate34RequestGarbageCollectionForTestingENS0_21GarbageCollectionTypeE + 118
12  iojs                                0x00000001001749ee _ZN2v88internal25FunctionCallbackArguments4CallEPFvRKNS_20FunctionCallbackInfoINS_5ValueEEEE + 158
13  iojs                                0x000000010019c97b _ZN2v88internalL19HandleApiCallHelperILb0EEENS0_11MaybeHandleINS0_6ObjectEEEPNS0_7IsolateERNS0_12_GLOBAL__N_116BuiltinArgumentsILNS0_21BuiltinExtraArgumentsE1EEE + 811
14  iojs                                0x000000010019f1cd _ZN2v88internalL21Builtin_HandleApiCallEiPPNS0_6ObjectEPNS0_7IsolateE + 61
15  ???                                 0x000005769a9060bb 0x0 + 6006957433019
16  ???                                 0x000005769a9fd9a2 0x0 + 6006958447010
17  ???                                 0x000005769a9fd6ff 0x0 + 6006958446335

Logs leading up to crash:

END TEST: gdal.Dataset instance "bands" property count() should throw if dataset is closed 
BEGIN TEST: gdal.Dataset instance "bands" property get() should return RasterBand 
Created Dataset [0x10100dfb0]
Created band [0x10100f4e0] (dataset = 0x10100dfb0)
Adding band to cache[0x10100f4e0] (parent=0x10100dfb0)
Starting garbage collection
Disposing band [0x10100f4e0]
Overviews [0x10100f4e0]
Mask [0x10100f4e0]
Disposed band [0x10100f4e0]
Disposing Datasource [0x10100dfb0]
Disposed Datasource [0x10100dfb0]
Disposing Dataset [0x10100dfb0]
Disposed Dataset [0x10100dfb0]
ObjectCache Weak Callback [0x10100dfb0]
Deleted Object Cache Item in Weak Callback [0x10100dfb0]
ObjectCache Weak Callback [0x10100f4e0]
Deleted Object Cache Item in Weak Callback [0x10100f4e0]
Finished garbage collection
END TEST: gdal.Dataset instance "bands" property get() should return RasterBand 
BEGIN TEST: gdal.Dataset instance "bands" property get() should return null if band id is out of range 
Created Dataset [0x10100e150]
Starting garbage collection
Disposing Datasource [0x10100e150]
Disposed Datasource [0x10100e150]
Disposing Dataset [0x10100e150]
Disposed Dataset [0x10100e150]
ObjectCache Weak Callback [0x10100e150]
Deleted Object Cache Item in Weak Callback [0x10100e150]
Finished garbage collection
END TEST: gdal.Dataset instance "bands" property get() should return null if band id is out of range 
BEGIN TEST: gdal.Dataset instance "bands" property get() should throw if dataset is closed 
Created Dataset [0x10100e150]
Disposing Datasource [0x10100e150]
Disposed Datasource [0x10100e150]
Disposing Dataset [0x10100e150]
Disposed Dataset [0x10100e150]
Starting garbage collection
ObjectCache Weak Callback [0x10100e150]
Deleted Object Cache Item in Weak Callback [0x10100e150]
Finished garbage collection
END TEST: gdal.Dataset instance "bands" property get() should throw if dataset is closed 
BEGIN TEST: gdal.Dataset instance "bands" property forEach() should call callback for each RasterBand 
Created Dataset [0x10100e150]
Created band [0x10100fcd0] (dataset = 0x10100e150)
Adding band to cache[0x10100fcd0] (parent=0x10100e150)
Starting garbage collection
Disposing Datasource [0x10100e150]
Disposed Datasource [0x10100e150]
Disposing Dataset [0x10100e150]
Unwrapping band [0x10100fcd0]
About to Unwrap Near Death handle [0x10100fcd0]

I've tried the js + node-weak workaround, but thats equally temperamental because more segfaults happen if you try to access certain things from the weak callback. I'll try to get back to this with fresh eyes in a few days. In the meantime does any V8::Persistent guru (aka @kkoopa) have any ideas of how to make this less of a tempermental mess?

[kkoopa]

I'm not entirely sure what's what here. Are you trying to access an object after its weak callback has executed? That is not allowed. The Persistent is cleared before the weak callback runs.

[brandonreavis]

Yeah its a bit of a mess. As it is now, the RasterBand object is accessed from the Dataset destructor.

The goal is that when a node_gdal::Dataset destructor is executed, it will check a map to see which of the child node_gdal::RasterBand objects are in javascript-land and then mark those as unusable (because the parent dataset is now closed).

Example:
Dataset object D1 has RasterBands B1 and B2. All three go out of scope and garbage collector first executes destructor of D1. B1 and B2 need to be properly disposed of before D1 is fully disposed, so the destructor of D1 goes to the cache and gets the weak Persistent handles of B1 and B2, unwraps them, and runs their dispose() methods. D1 can then be fully closed. The weak callback has not been called for B1 or B2 when im trying to unwrap them, but they are "near death" the crash happens.

Its entirely possible that im looking at this problem the wrong way.

[kkoopa]

Then something weird is going on. The callback should have been called when the handles were near death, it is not allowed to access them in that state.

When a handle is made weak its state is set to WEAK https://github.com/nodejs/node/blob/master/deps/v8/src/global-handles.cc#L244
from there it will go to PENDING and then to NEAR_DEATH https://github.com/nodejs/node/blob/master/deps/v8/src/global-handles.cc#L292 or
https://github.com/nodejs/node/blob/master/deps/v8/src/global-handles.cc#L302

    WEAK,        // Flagged as weak but not yet finalized.
    PENDING,     // Has been recognized as only reachable by weak handles.
    NEAR_DEATH,  // Callback has informed the handle is near death.

[brandonreavis]

Thanks @kkoopa. Turns out IsNearDeath() returns true if the state is NEAR_DEATH or PENDING here. If the handle was PENDING it explains why the weak callback wasn't called yet... but it was ready to be called. It looks like its not safe to use handles that are pending because CollectPhantomCallbackData() zaps them with something dangerous. The previous version of NAN worked because because the weakness type was NORMAL_WEAK instead of PHANTOM_WEAK so this method wasn't called. This is my understanding at least.

I ended up restructuring things so it never has to deal with any V8 handles in the destructors / weak callbacks which should make it safer and easier to follow what is happening where.

[brandonreavis]

The only remaining thing to do is fix the TypedArrays used for reading and writing band pixels. TypedArray support doesn't exist in Nan but that may soon change with @mikolalysenko's recent pull request nodejs/nan#459. So I'm waiting to see how that goes.

[kkoopa]

Yes, your understanding is correct. NORMAL_WEAK is no more, it's all phantom callbacks nowadays.

[weagle08]

Has this been addressed so that we can build node-gdal against node v4.x?

[brianreavis]

Yes, this is complete – with the caveat being Windows (see #128)