rewrite on node-api

[Julusian]

These changes are published as https://www.npmjs.com/package/@julusian/midi, and is currently 100% api compatible

I have a project which will soon need midi support in nodejs, and so have been slowly working on porting it to node-api and preparing for bundling prebuilds in the npm package. There is some junk in this PR currently, I shall remove that once I hear from a maintainer that they would like to review/accept this PR.

I already have a growing collection of node-api based native modules that I maintain, are you interested in another maintainer here? I am happy to simply work on getting this PR merged, but maintenance here looks very quiet recently, it looks like you could do with some help.

I am happy that these changes have been tested on windows, mac and linux. Of course there could still be bugs, if anyone finds something do let me know!

[hrueger]

Hi @Julusian, Thanks for your work 😀 I'm happy to test this as I'm very interested in using it myself. Do you have beta versions on npm? Does this also make the package context-aware so that it can be used in a worker thread for example?

[Julusian]

Hey @hrueger I have just published this on npm https://www.npmjs.com/package/@julusian/midi. So far I am happy that both input and output are working on linux, I havent tried running it elsewhere, but it does build. I also havent checked for memory leaks or weird crashes yet. Yes, node-api based modules have to be context-aware, so this will work happily with worker_threads. There was one part of the previous version of this module which was not thread safe (even after applying #213 or #214), which has been resolved in this.

[hrueger]

Thanks a lot. I've tested it with my fork of easymidi: https://github.com/hrueger/node-easymidi I've tried the examples on Windows (without Virtual Ports as they are not available unfortunately) as well as on MacOS. I did not have any issues. I only tested it using Node tough. I'll try Electron + worker_threads soon, currently the build is breaking because I'm still using usb-detection and I need to migrate to node-usb first.

[hrueger]

Hi @Julusian, I was able to test it in Electron now. At first, it worked perfectly, but then I started to see some crashes occasionally. Does this stack trace tell you anything? https://sentry.internal.makepro-x.com/share/issue/90d7bda70fc24e49a3c4f1cd9e3051df/

I'm pretty sure that this comes from the midi lib, because if I inspect c1a2c659-3a32-4012-a1e8-2a3fce594131.tmp.node using any text editor, I find some ASCII text about Midi:

[Julusian]

no that stack trace doesnt really say anything useful..

what version of electron are you using? and it looks like this is happening on windows for you? any chance of a script which I can run to reproduce the issue?

[hrueger]

It's Electron 20.1.4 and yes, I've been using windows there. I'll try to make a simple repo script.

[Julusian]

ok, I suspect this is hitting https://www.electronjs.org/blog/v8-memory-cage. It looks like this is wrapping an external pointer in a buffer https://github.com/Julusian/node-midi/blob/master/src/input.cpp#L120, which is precisely what they say to avoid doing https://www.electronjs.org/blog/v8-memory-cage#how-will-i-know-if-my-app-is-impacted-by-this-change.

But Im surprised it only crashes occasionally. Would you be able to give it a try with electron 19? If it doesnt crash there, then that would confirm this to be the issue

If this is the cause of the crash, then you might have other issues, as that 'rare' pattern does not look that rare to me (eg https://github.com/node-hid/node-hid/blob/master/src/HID.cc#L166 https://github.com/Julusian/node-jpeg-turbo/blob/master/src/compress.cc#L68)

[hrueger]

Interesting. Sure, I'll try electron 19 but I don't think I'll get to that today. I haven't seen a crash regarding node-hid or jpeg-turbo so far, but I also didn't connect a stream deck. I'll test that 👍

[Julusian]

Ignore that idea, after quite some digging, it turns out that the memory-cage change applies to v21 not v20 like it is documented to be.

Ill give the code a read and do a preemptive fix for electron 21, but a reproduction script would be really helpful

[hrueger]

https://sentry.internal.makepro-x.com/share/issue/b2dead00224847c4aea486ecd5b56f02/

I was able to upload the debug files to Sentry and trigger the bug again. Can you find anything useful in that stack trace now?

[Julusian]

yeah, that stack trace is a lot more meaningful now, but the place it is crashing is rather odd.. Ill have a play and see what I can figure out from there

[hrueger]

Good to hear. I'm trying to prepare a simple reproduction repo right now. I also had a problem with errors just saying "Internal RTPMidi Error" when trying to close and reopen ports. I'll get back to you soon.

[hrueger]

I think I forgot to tell you that I'm using it in a worker_thread as well as in the main thread. Hopefully I can recreate that in the test case repo.

[hrueger]

I just noticed that the midi.d.ts file is missing in the tarball: https://registry.npmjs.org/@julusian/midi/-/midi-3.0.0-2.tgz and therefore also in node_modules when installed...

[hrueger]

OK, I finally got it crashing 😉

Please check out this repository: https://github.com/hrueger/midi-napi-tests

1. Install dependencies using yarn
2. Start the example scripts with npm start [name of problem script]

There are two problem scripts currently:

1. no-error-with-multiple-connections: This is something which I believe behaves similarily to the original midi library but could be improved. Basically, If you already opened a port and try to open another one, it just prints a log to the console (MidiInWinMM::openPort: a valid connection already exists! with one or two weird newlines) and does not throw an error. This is, however, not as important as number 2:
2. crash: This crashes after two seconds when I terminate the worker thread. Unfortunately, it doesn't print an error to the console, but if you comment out the midi stuff, it works just fine. Also, if I add Sentry, I see the error online.

Does that help? If I can do anything to help debugging, just tell me.

[Julusian]

1) Yeah, I can see that. It looks like the midi library being wrapped classifies that as a warning and so only outputs it to the console..

2) Unfortunately it isnt crashing for me. I've tried it both on Windows 10 and Ubuntu 20.04, and it happily sits there for forever

I don't know how to proceed right now. Perhaps I need to produce a build which does a lot of console logging to give an indication of where it got to?

[Julusian]

Note for myself: Maybe rtmidi should we replaced with https://github.com/jcelerier/libremidi? It looks to be a bit more modern, and although it suffers a lot of the same flaws, has more activity so is more likely to get fixes included?

[hrueger]

Unfortunately it isnt crashing for me. I've tried it both on Windows 10 and Ubuntu 20.04, and it happily sits there for forever

Hm, how many Midi Ports do you have on your system? I have about 8 created with loopMidi and a couple more from rtpMidi.

It happens on windows for me, I don't have Ubuntu available to test, but I can try on the Mac tomorrow.

[hrueger]

Note for myself: Maybe rtmidi should we replaced with https://github.com/jcelerier/libremidi? It looks to be a bit more modern, and although it suffers a lot of the same flaws, has more activity so is more likely to get fixes included?

That looks interesting, indeed. I wonder if that can do virtual Midi Ports on Windows? That would be really cool, because people would not have to use loopMidi to create a virtual port in order to connect two programs using Midi. The only thing I have found so far which can do that is the "base" of loopMidi: https://www.tobias-erichsen.de/software/virtualmidi.html However, this is not open-source unfortunately.

[Julusian]

Hm, how many Midi Ports do you have on your system? I have about 8 created with loopMidi and a couple more from rtpMidi.

Ah, that did it. I had a couple with loopmidi, but increasing that to 18 triggers it

[Julusian]

@hrueger Please give v3.0.0-3 a try. It is no longer crashing for me in your reproduction. It also includes the typescript typings too

[hrueger]

Thanks a lot, that sounds great. I'll test that early tomorrow.

[hrueger]

That fixed the crash 👍 Thanks a lot.