baudehlo / node-fs-ext

Extras missing from node's fs module
MIT License
101 stars 45 forks source link

Segmentation fault in uv_ref #21

Closed ghost closed 11 years ago

ghost commented 11 years ago

Hi; just wanted to leave a note here. I'm seeing a random crash (maybe one in every five executions) of a program that's using flock via node-fs-ext:

Program terminated with signal 11, Segmentation fault.
#0  0x0820323d in uv_ref (handle=0xb9148cd4) at ../deps/uv/src/uv-common.c:420
420       uv__handle_ref(handle);
(gdb) bt
#0  0x0820323d in uv_ref (handle=0xb9148cd4) at ../deps/uv/src/uv-common.c:420
#1  0xa115404c in Flock(v8::Arguments const&) ()
   from /srv/software/medic-core/v1.2.1/x86/lib/node_modules/fs-ext/build/Release/fs-ext.node
#2  0x9f34cf49 in ?? ()
#3  0x9f34c953 in ?? ()

I'm going to see if I can figure this out; obviously any ideas would be greatly appreciated.

ghost commented 11 years ago

On Node v0.10.1, but can reproduce the issue on Node v0.8.22 also.

ghost commented 11 years ago

I think this is actually my fault; everything I'm reading says that an explicit uv_ref isn't necessary after uv_queue_work(uv_default_loop(), ...). Removing the uv_ref lines appears to fix the issue. I'm going to read more docs and test it some more to make sure nothing's leaking; and then I'll send a pull request.

ghost commented 11 years ago

Here's an explanation that says the uv_ref / uv_unref are not necessary when used with uv_queue_work(uv_default_loop(), ...): https://github.com/developmentseed/node-sqlite3/pull/76

However, here's a commit that uses uv_ref after uv_queue_work(uv_default_loop(), ...): https://github.com/kashif/node-geos/blob/master/src/cpp/geometry.hpp

@baudehlo - I'm obviously pretty new to this; do you know the right way off the top of your head? Thanks.

ghost commented 11 years ago

Merged; I think I got it. Leaving this here for documentation and so @baudehlo can review if he wants.

baudehlo commented 11 years ago

Sounds good to me - I'm all pretty new to it too. As long as it doesn't leak memory, I'm happy.

baudehlo commented 11 years ago

I'll update package.json and do a release when I get to work tomorrow.