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.

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

Remove unmatched uv_unref() causing segfault #32

Merged
merged 1 commit into from
Jan 21, 2014

Conversation

drewish
Copy link
Collaborator

@drewish drewish commented Dec 14, 2013

Fixes #16

The tests seem to hang but I'm not sure if that's normal: Tests now run correctly

amorton@pavlov:~/projects/node-midi% npm test

> [email protected] test /Users/amorton/projects/node-midi
> node test/virtual-loopback-test-automated.js

Enumerating inputs
Input found: MidiKeys
Input found: node-midi Virtual Output
Opening node-midi Virtual Output
Enumerating outputs
Output found: MidiKeys
Opening node-midi Virtual Input
Sending message
Virtual input recieved m:144,23,81 d:0
Input recieved m:144,33,81 d:0
Sending message
Virtual input recieved m:144,23,81 d:1.000385828
Input recieved m:144,33,81 d:0.999496216
Sending message
Virtual input recieved m:144,23,81 d:1.0010956880000002
Input recieved m:144,33,81 d:1.0011307120000001
Sending message
Virtual input recieved m:144,23,81 d:1.001120228
Input recieved m:144,33,81 d:1.000997857
Sending message
Virtual input recieved m:144,23,81 d:1.001917765
Input recieved m:144,33,81 d:1.002010969
Sending message
Virtual input recieved m:144,23,81 d:1.000205188
Input recieved m:144,33,81 d:1.0002009040000002
Sending message
Virtual input recieved m:144,23,81 d:1.00220642
Input recieved m:144,33,81 d:1.002208001
Sending message
Virtual input recieved m:144,23,81 d:1.002072784
Input recieved m:144,33,81 d:1.00208373
Sending message
Virtual input recieved m:144,23,81 d:1.0011529670000001
Input recieved m:144,33,81 d:1.0012620220000001
amorton@pavlov:~/projects/node-midi% 

I'll try building it on 0.8.x and see what the tests do there.

Also, properly close the async watcher when closing the port.

Fixes justinlatimer#16
@drewish
Copy link
Collaborator Author

drewish commented Dec 14, 2013

So per https://github.com/joyent/libuv/blob/master/include/uv.h#L1287 it sounds like the watcher needs to be explicitly closed. Once I added that the tests run and exit normally.

@drewish
Copy link
Collaborator Author

drewish commented Jan 18, 2014

I just tested this branch on Ubuntu 13.04 with Node 0.10.24 and hit the following:

node: ../src/node_object_wrap.h:104: virtual void node::ObjectWrap::Unref(): Assertion `!handle_.IsWeak()' failed.
Aborted (core dumped)

Stacktrace:

 #0  0x00007fa284e5e037 in raise () from /lib/x86_64-linux-gnu/libc.so.6
 No symbol table info available.
 #1  0x00007fa284e61698 in abort () from /lib/x86_64-linux-gnu/libc.so.6
 No symbol table info available.
 #2  0x00007fa284e56e03 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
 No symbol table info available.
 #3  0x00007fa284e56eb2 in __assert_fail () from /lib/x86_64-linux-gnu/libc.so.6
 No symbol table info available.
 #4  0x000000000081b772 in node::ObjectWrap::Unref() ()
 No symbol table info available.
 #5  0x00007fa284c21bdc in NodeMidiInput::ClosePort(v8::Arguments const&) () from /home/parallels/node-midi/build/Release/midi.node
 No symbol table info available.

So I'll dig into that more.

@drewish
Copy link
Collaborator Author

drewish commented Jan 19, 2014

Okay so a lot of digging in to the Linux side and it looks like the issue is that on the ALSA side when opening the virtual port it uses the client name (RtMidi Output Client) instead of the passed in port name (node-midi Virtual Output) and since the name doesn't match the port is never opened. Then when we try to close it, it's really running into the closing a closed port: #21

I think this patch is still good to go.

@drewish
Copy link
Collaborator Author

drewish commented Jan 19, 2014

For reference here's the output of the tests:

> node test/virtual-loopback-test-automated.js

Enumerating inputs
Input found: Midi Through:0
Input found: RtMidi Output Client:0
Enumerating outputs
Output found: Midi Through:0
Output found: RtMidi Output Client:0
Sending message
Sending message
Sending message
Sending message
Sending message
Sending message
Sending message
Sending message
Sending message
node: ../src/node_object_wrap.h:104: virtual void node::ObjectWrap::Unref(): Assertion `!handle_.IsWeak()' failed.
Aborted (core dumped)
npm ERR! Test failed.  See above for more details.
npm ERR! not ok code 0
parallels@ubuntu:/media/psf/Home/tmp/node-midi$ 

Compare it to the run at the top and you'll see the port names don't match.

@drewish
Copy link
Collaborator Author

drewish commented Jan 19, 2014

I opened up thestk/rtmidi#6 to try to address the return value of getPortName() under ALSA.

@justinlatimer
Copy link
Owner

This looks good.

Before merging I'd like to confirm that this still works with older versions of uv, i.e. node-08. If not, we need to change the package engine versions for the correct versions of node that work.

Thanks!

@drewish
Copy link
Collaborator Author

drewish commented Jan 20, 2014

Yeah it works fine in 0.8.x:

andrew@humming:~/tmp/node-midi% rm -rf build/
andrew@humming:~/tmp/node-midi% node -v
v0.8.22
andrew@humming:~/tmp/node-midi% npm install

> [email protected] install /Users/andrew/tmp/node-midi
> node-gyp rebuild

  CXX(target) Release/obj.target/midi/src/node-midi.o
In file included from ../src/node-midi.cpp:8:
../src/lib/RtMidi/RtMidi.cpp:473:26: warning: implicit conversion of NULL constant to 'MIDIEntityRef' (aka 'unsigned int')
      [-Wnull-conversion]
  MIDIEntityRef entity = NULL;
                ~~~~~~   ^~~~
                         0
1 warning generated.
  SOLINK_MODULE(target) Release/midi.node
  SOLINK_MODULE(target) Release/midi.node: Finished
andrew@humming:~/tmp/node-midi% npm test

> [email protected] test /Users/andrew/tmp/node-midi
> node test/virtual-loopback-test-automated.js

Enumerating inputs
Input found: node-midi Virtual Output
Opening node-midi Virtual Output
Enumerating outputs
Output found: node-midi Virtual Output
Opening node-midi Virtual Input
Sending message
Virtual input recieved m:144,23,81 d:0
Input recieved m:144,33,81 d:0
Sending message
Virtual input recieved m:144,23,81 d:1.000639612
Input recieved m:144,33,81 d:1.000462054
Sending message
Virtual input recieved m:144,23,81 d:0.999677129
Input recieved m:144,33,81 d:0.999563101
Sending message
Virtual input recieved m:144,23,81 d:1.000710055
Input recieved m:144,33,81 d:1.000723075
Sending message
Virtual input recieved m:144,23,81 d:1.000833938
Input recieved m:144,33,81 d:1.000774663
Sending message
Virtual input recieved m:144,23,81 d:1.0015966790000002
Input recieved m:144,33,81 d:1.0016236170000001
Sending message
Virtual input recieved m:144,23,81 d:1.000546744
Input recieved m:144,33,81 d:1.0009048390000002
Sending message
Virtual input recieved m:144,23,81 d:1.000116003
Input recieved m:144,33,81 d:0.999866137
Sending message
Virtual input recieved m:144,23,81 d:1.000719822
Input recieved m:144,33,81 d:1.000615009
Sending message
Virtual input recieved m:144,23,81 d:1.001503491
Input recieved m:144,33,81 d:1.0015179490000001
andrew@humming:~/tmp/node-midi% 

justinlatimer added a commit that referenced this pull request Jan 21, 2014
Remove unmatched uv_unref() causing segfault
@justinlatimer justinlatimer merged commit 58aaeca into justinlatimer:master Jan 21, 2014
@drewish drewish deleted the node-0.10 branch February 22, 2015 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to node 0.10.x segfaults
2 participants