search

Julusian / node-midi

A node.js wrapper for RtMidi providing MIDI I/O
https://www.npmjs.com/package/@julusian/midi
MIT License
22 stars 7 forks source link

Allow release of Input Output objects? #7

Closed halfbyte closed 1 year ago

halfbyte commented 1 year ago

We're (Focusrite/Novation) currently using a fork of the original node-midi library and using your modernised fork sounds like a good idea to be more future proof.

We have one very particular usecase which is that we use the virtual ports to test our software and that includes cases where we wait for devices to connect, disconnect and reconnect (because, among other things, we do firmware upgrades via sysex).

The main issue here is that closePort() does not make the virtual port go away, for that the RTMidiInput/Output objects needs to be discarded. Nulling the input object in JavaScript land will probably free the device eventually but that's not usable for our usecase.

In our case, we have added a release() method to the input/output objects that free the RTMidi objects. This works fine for our usecase but leads to a slightly odd internal state after the release.

I have experimented with your package and can prepare a PR that would show what I mean (It's basically just calling handle.reset()) but I also wanted to ask if maybe you have a better idea of how to properly release everything in a way that doesn't rely on a GC run.

Julusian commented 1 year ago

It feels like this is a flaw/bug in my implementation of closePort(), and that they should be doing a handle.reset() too.
Every other method that uses the handle looks to be already doing a null check (including resetPort), so it looks like I intended it to do that cleanup as part of closePort, but didnt notice that it was missing.

I will happily accept a PR to improve upon what it is currently doing.

halfbyte commented 1 year ago

@Julusian Hm, interesting. I guess coupling it to closePort() would work for us, but the downside of that is that you cannot open the port again, which would work atm. (There's even a (manual) test for that in test/input/input-reopen-test.js that would fail.

I'll create the PR and we can discuss this there. Thanks for the quick feedback!

assaron commented 1 year ago

Hi, what's the status of this issue? I also stumbled open this problem of virtual ports cluttering up.

In the similar python package they ended up with adding a delete method: https://github.com/SpotlightKid/python-rtmidi/issues/60, so I guess the original suggestion by @halfbyte can be accepted?

Julusian commented 1 year ago

This is published as 3.1.0, with a destroy() method now available

assaron commented 1 year ago

Cool, thanks!