search

DerekCook / CoreMidi4J

Core MIDI Service provider Interace (SPI) for Java 1.7 and above on OS X
Eclipse Public License 1.0
54 stars 15 forks source link

CoreMIDI4J doesn't like repeatedly disconnecting devices #19

Closed eclab closed 7 years ago

eclab commented 7 years ago

I'm getting a lot of crashes after disconnecting devices and reconnecting again (perhaps 3 or 4 times -- not many!). This is in Edisyn (https://github.com/eclab/edisyn/), from selecting the "MIDI:Change MIDI" menu 3 or 4 times. Try it out! I'm running Java 1.7.0_51 on OS X 10.9.5. I'll usually get a hang or a hard VM dump. Example VM dump stack trace:

uk.co.xfactorylibrarians.coremidi4j.CoreMidiException: Exception in CoreMIDI JNI Library by "MIDIPortDisconnectSource" - OS STatus Code: ffffd5af
    at uk.co.xfactorylibrarians.coremidi4j.CoreMidiInputPort.midiPortDisconnectSource(Native Method)
    at uk.co.xfactorylibrarians.coremidi4j.CoreMidiInputPort.disconnectSource(CoreMidiInputPort.java:71)
    at uk.co.xfactorylibrarians.coremidi4j.CoreMidiSource.close(CoreMidiSource.java:130)
    at edisyn.Synth.gatherMidiDevices(Synth.java:1203)
    at edisyn.Synth.setupMIDI(Synth.java:486)
    at edisyn.Synth.setupMIDI(Synth.java:468)
    at edisyn.Synth$14.actionPerformed(Synth.java:780)
    at javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:2018)
    at javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2341)
    at javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:402)
    at javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:259)
    at javax.swing.AbstractButton.doClick(AbstractButton.java:376)
    at com.apple.laf.ScreenMenuItem.actionPerformed(ScreenMenuItem.java:128)
    at java.awt.MenuItem.processActionEvent(MenuItem.java:669)
    at java.awt.MenuItem.processEvent(MenuItem.java:628)
    at java.awt.MenuComponent.dispatchEventImpl(MenuComponent.java:351)
    at java.awt.MenuComponent.dispatchEvent(MenuComponent.java:339)
    at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:738)
    at java.awt.EventQueue.access$200(EventQueue.java:103)
    at java.awt.EventQueue$3.run(EventQueue.java:694)
    at java.awt.EventQueue$3.run(EventQueue.java:692)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
    at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:87)
    at java.awt.EventQueue$4.run(EventQueue.java:708)
    at java.awt.EventQueue$4.run(EventQueue.java:706)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
    at java.awt.EventQueue.dispatchEvent(EventQueue.java:705)
    at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:242)
    at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:161)
    at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:150)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:146)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:138)
    at java.awt.EventDispatchThread.run(EventDispatchThread.java:91)
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGBUS (0xa) at pc=0x00007fff754e8340, pid=18392, tid=77059
#
# JRE version: Java(TM) SE Runtime Environment (7.0_51-b13) (build 1.7.0_51-b13)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (24.51-b03 mixed mode bsd-amd64 compressed oops)
# Problematic frame:
# C  [CoreFoundation+0xe6899340]  OBJC_METACLASS_$___NSCFString+0x0
#
# Failed to write core dump. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
#
# An error report file with more information is saved as:
# /Users/sean/java/distros/edisyn.git/trunk/hs_err_pid18392.log
#
# If you would like to submit a bug report, please visit:
#   http://bugreport.sun.com/bugreport/crash.jsp
#
eclab commented 7 years ago

Forgot to include the err log. err.log.zip

eclab commented 7 years ago

An additional bug, but one that I cannot produce a log for. It looks like you can't dynamically allocate receivers.

  1. I allocate three devices via uk.co.xfactorylibrarians.coremidi4j.CoreMidiDeviceProvider.getMidiDeviceInfo();

  2. I open a transmitter on one device, and a receiver on two others.

  3. I use them a bit, then close the receivers and transmitters.

  4. I then open a new transmitter on one device, and a receiver on two others.

  5. On opening one of the receivers, the entire system reliably hangs hard and permanently.

Due to this and the previous bug, at present I have no choice but to create devices initially, then create hand-coded pipes for the receivers and transmitters that manage opening and closing. This works but means that I cannot dynamically handle new devices coming online if the user plugs a synth into his computer after the software has been launched.

DerekCook commented 7 years ago

Hi, Sorry for the delay in replying. I am away on business and working manic hours, so not sure when I will get to look at this. It would be helpful if you can provide some sample code that guarantees both of the crashes you are talking about to give us something to work with in addition to the descriptions that you have provided.

brunchboy commented 7 years ago

Yes, sorry, I am incredibly buried with my other projects as well, and don’t know when I might have time to look at issues like this, but sample code that reproduces it reliably will definitely be key. Derek, if you do happen to find and fix it, I will of course make time to publish a new release.

BradleyRoss commented 7 years ago

Just a thought. When you close all the transmitters and receivers, make sure that the devices are open before opening the new transmitters and receivers. (MidiDevice.isOpen()) I saw something that indicated that the devices may close if the receivers and transmitters are closed. I'm curious and will try running your project.

eclab commented 7 years ago

I'll try to get you guys a simple piece of example code [I've since modified my program heavily to work around the issue, so it won't be of much use to you except in early versions on github].

Sean

On Jun 6, 2017, at 7:22 AM, James Elliott notifications@github.com wrote:

Yes, sorry, I am incredibly buried with my other projects as well, and don’t know when I might have time to look at issues like this, but sample code that reproduces it reliably will definitely be key. Derek, if you do happen to find and fix it, I will of course make time to publish a new release.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

eclab commented 7 years ago

Example.zip

On Jun 6, 2017, at 7:22 AM, James Elliott notifications@github.com wrote:

Yes, sorry, I am incredibly buried with my other projects as well, and don’t know when I might have time to look at issues like this, but sample code that reproduces it reliably will definitely be key. Derek, if you do happen to find and fix it, I will of course make time to publish a new release.

I am attaching an example stripped out of older code I had. It's not minimal but should be enough to demonstrate what's going on. The code gathers all the available MIDI devices for a receiver and two transmitters, then attaches receivers to the transmitters. It does this three times. It then closes everything cleanly, at which point it crashes hard. The code should be closing the existing receivers/transmitters and devices when it opens new ones. I could try stripping it down further if need be.

BradleyRoss commented 7 years ago

To eclab

I don't see the attached code

eclab commented 7 years ago

Sorry, it looks like github silently removes files with "unsupported extensions" from email messages. :-( And for some reason I cannot fathom, .java is an unsupported extension. Try now.

BradleyRoss commented 7 years ago

For one thing, you are testing if the various objects are not null, but I think you also need to check if the devices, transmitters, and receivers are open. The synchronized lock doesn't appear right. See http://winterbe.com/posts/2015/04/30/java8-concurrency-tutorial-synchronized-locks-examples/ and the java.util.concurrent.locks package. It looks like it should be synchronized(this) according to the winterbe.com blog post.

I'm going to have to look at this in more detail, but that is the result of my first look.

See https://stackoverflow.com/questions/10963205/how-to-attach-file-to-a-github-issue It appears that the acceptable file types are PNG, GIF, JPG, DOCX, PPTX, XLSX, TXT, or PDF

eclab commented 7 years ago

On Jun 20, 2017, at 3:08 AM, Bradley Ross notifications@github.com wrote:

For one thing, you are testing if the various objects are not null, but I think you also need to check if the devices, transmitters, and receivers are open.

They are null because I set them to be null immediately after closing them to make it clear that they're closed. I think the code's right.

The synchronized lock doesn't appear right.

You can comment out all the synchronized statements in the code because it's only single-threaded (its a remnant of my main code) but it won't make any difference.

I'm pretty sure the synchronization code is correct. Locking on a lock object other than [this] is good practice for many purposes, particularly if you need to have independent locks for different methods. Among the best choices for lock objects is an empty array because it is serializable (many other objects, including new Object(), are not).

DerekCook commented 7 years ago

Hi, Apologies for lack of response. I am am away on business and currently rammed. Any free time I have I have on programming is currently spent resolving an issue I have with one of my Java Librarians that use CoreMIDI4J. Will look at this as soon as I can, but if you can find the issue and generate a pull request then I am sure that James and I will be able to respond to that much more quickly. I am happy to consider extending who can actively contribute as well if you have the interest. CoreMIDI4J was put here so it could be supported unlike the older providers..

brunchboy commented 7 years ago

I should have some time to play with this over the weekend too, I hope. My massive updates to dysentery, beat-link, and beat-link-trigger are out in preview form, and feedback is settling down into a happy place.

brunchboy commented 7 years ago

So I have been poking a bit at this, and do not have a definitive answer yet. I discovered that the example was crashing trying to free a bit of memory that was not allocated, in CoreMidiInputPort.cpp at line 261, free((void *) memoryReference);

Now, I am not going to be nearly as efficient at debugging this as Derek might because I did not create these classes and I don’t understand the details of their relationships yet, but I did notice a few things that made me go “hmm.” The open() and close() methods in CoreMidiSource do not protect themselves against being called more than once, and in those circumstances will try to free the same block of memory more than one time, which would lead to this kind of crash.

Indeed, I just tried adding protection against this, and the crash has gone away, the example program has been running with no problems for twenty minutes now. I will want to go through the other classes and add similar protections, but this seems a step in the right direction. In the mean time I will make a preview release with this fix in it and see if it addresses Sean’s ( @eclab ) issue at all.

@DerekCook I also notice that the close() method in CoreMidiSource simply clears the list of transmitters; should it not also close them?

brunchboy commented 7 years ago

OK, @eclab here is a preview version with this fix present, can you see if it behaves better for you? I have to zip it up, as we noted earlier in the discussion.

I have removed this earlier snapshot because it has been superseded by a newer one below.

brunchboy commented 7 years ago

@DerekCook I fixed a couple of places where we were returning NULL when we should have been returning an empty list according to the SPI spec as well. But I also see that we are not maintaining a list of receivers in CoreMidiDestination (the comment says “We do not maintain a list of receivers, as they tend to be transitory context”). To properly implement the MIDI SPI spec, we should probably go ahead and add this feature. We are also supposed to remove receivers and transmitters from our lists when they are closed. Doesn’t CoreMidi keep these lists for us already, perhaps?

brunchboy commented 7 years ago

Everyone, here is a newer snapshot in which I have gone ahead and implemented the rest of the things we are supposed to do in terms of tracking transmitters and receivers that we create, as well as removing them when they get closed, and closing them when their parent MIDI device closes. This makes us much more correctly implement the MIDI Service Provider Interface contract, and will hopefully fix the problems @eclab was seeing. @DerekCook , this is enough of a change that you probably want to test it with some of your synths as well. But if it looks good to everyone, we should probably make another release with it soon.

Again, snapshot removed because there is a newer one below.

brunchboy commented 7 years ago

And here is one more (last, I hope?) snapshot version to try. I further noticed that we were not closing our devices when we notice them disappearing from the MIDI environment. This cleans that up, so that if anyone is holding on to a device reference for something that has been detached from the MIDI environment (or any receivers or transmitters associated with such a device), they will properly be marked as closed, and give you appropriate exceptions when you try to use them at that point. Again, you will have to unzip this in order to test it. coremidi4j-1.1-SNAPSHOT.jar.zip

eclab commented 7 years ago

All, I'm going to have exceptionally poor internet access for about a month due to vacation location. I'll try to do a bit of testing but the turnaround could be weeks. Thanks for getting on this.

Sean

brunchboy commented 7 years ago

No worries! Our turnaround was not particularly fast in responding to your report, so that is only fair. 😁 And I suspect it may be a while before Derek has time to do his testing too.

DerekCook commented 7 years ago

James, Thanks very much for getting here first. I of course trust your judgement in what the correct behaviour should be, and the focus in the early days was getting something that worked that undoubtedly would need improvement!

I will be back home soon, and will be doing a new release of my librarians as a have quite a few fixes batched up (have been able to work on them whilst away on a laptop (but no synths and MIDI devices), so will need to do some synth testing, and I will do that with then new release of CoreMIDI4J.

brunchboy commented 7 years ago

That sounds perfect. And of course, I understand well the challenge of getting this library working at all, and the plan of polishing rough edges as we find them! I was so daunted by the prospect of the initial implementation that I waited for you to come along and do it. I’m glad to be able to contribute a bit from time to time.

DerekCook commented 7 years ago

Have tried the new release with all of my librarian and synth combinations, and all looks good. But suggest we keep this open until we hear from Sean.

brunchboy commented 7 years ago

Thanks, that’s great (and fast)! And I agree.

brunchboy commented 7 years ago

@eclab I hope you had a lovely vacation, and will have a chance to test this fix.

eclab commented 7 years ago

Not back yet! Back in a week. So you have to give me a few. :-)

On Aug 5, 2017, at 9:53 PM, James Elliott notifications@github.com wrote:

@eclab I hope you had a lovely vacation, and will have a chance to test this fix.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

DerekCook commented 7 years ago

Hi, when you get time, please give this a go and let us know how it works for you.

eclab commented 7 years ago

I'm ready to test the new version of the code: but it looks like there's no jar file available, I'd have to build it from source (which I'm not set up to do). Can someone build a jar of the current code, meant for handling issue #19, and stick it somewhere I can grab?

brunchboy commented 7 years ago

If you scroll up a bit you will find it in this thread, attached to my comment on July 2.

eclab commented 7 years ago

Aaaaand I am a big dummy. Will get on it.

Sean

On Aug 27, 2017, at 9:44 PM, James Elliott notifications@github.com wrote:

If you scroll up a bit you will find it in this thread, attached to my comment on July 2.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

brunchboy commented 7 years ago

Not at all! There was a lot of other stuff discussed since then, it was very easy to miss.

eclab commented 7 years ago

I am happy to report that I cannot reproduce the errors at this point using the build snapshot above. I have tested both with the test code I submitted earlier and with production code in Edisyn.

brunchboy commented 7 years ago

That’s excellent news, thanks so much for testing and reporting that. Derek, should I make a release with these fixes, or should we wait until you have a chance to finish the other enhancements you are working on as well?

DerekCook commented 7 years ago

Hi, James.

I think I am done with the changes I plan to do on #21

Haven't looked at #22 yet (I really need to get back to some other things for a while), and one of the reasons for using your Maven build was to see if I got the problem in #20, which I didn't

So I think a combined release that gets #19 and #21, once others have had a chance to test, would make sense.

eclab commented 7 years ago

Any ETA on when a release might happen? I'd like to release a new version of Edisyn, but I'd prefer to wait so I can release it with the new version of CoreMIDI4J.

brunchboy commented 7 years ago

I could push a release tonight if Derek feels it is ready. Has anyone had a chance to test the snapshot that Derek posted in the discussion for issue #21? @DerekCook, there haven’t been any changes since then, have there?

DerekCook commented 7 years ago

I'm happy if people are happy with their own testing after #21 and the snapshot on it, which provides new device names - it works for me. I have done no changes since my last commit, so the Master Branch is up to date with my changes for #19 and #21

brunchboy commented 7 years ago

All right, I will prep it for release, and try Beat Link Trigger with a local copy. I will also update a few of the Maven plugins we use, which have had releases this past week, and if all goes well, will release it. @eclab did you have a chance to test the build in the issue #21 discussion?

brunchboy commented 7 years ago

All right, I definitely like the way you can now see the user-assigned device device names, as well as the combined device and port names when there are multiple ports for a device. One thing that is a little disconcerting, although I don’t know if there is anything we can do about it, is that changing device names in Audio MIDI Setup does not pass those changes through to Java until the device is disconnected and reconnected, or the JVM is shut down and relaunched. There may just not be an event to notify us of such changes? This is particularly confusing in the case where I start with just one port defined in the IAC Driver (“Bus 1”), which shows up in CoreMidi4J with the name “IAC Driver”. If I then add another port, “Bus 2”, CoreMIDI4J shows that I have two devices "IAC Driver” and “IAC Driver Bus 2”. Both of them are selectable and work. But if I then take the IAC Driver device offline and back online, or quit and restart the JVM, suddenly the names are, more sensibly “IAC Driver Bus 1” and “IAC Driver Bus 2”.

Since I save my port connections by name, that means that a configuration that works in one run will break the next time I run the program, simply because I added a second port to the IAC driver, which is likely to confuse me or my users.

Looking a bit deeper, I see the following comment in CoreMidiClient.cpp for notifyCallback:

// Message IDs (enum MIDINotificationMessageID)
//      1 kMIDIMsgSetupChanged
//      2 kMIDIMsgObjectAdded
//      3 kMIDIMsgObjectRemoved
//      4 kMIDIMsgPropertyChanged
//
//  In experimenting with notification of changes, you get four messages, in the sequence 4, 3, 3, 1 when removing an interface and 4, 2, 2, 1 when adding an interface
//  I am guessing that the duplicate 3, 3 or 2, 2 is because the deves in question have a source and a destination.
//
//  I think we only need to react to one message, the final one, not all four, assuming that we will use this as a trigger to rescan the interfaces.

And I think this means we can fix this problem. Now that we are affected by property changes (the renaming of devices), we should let Java know when we receive those as well as when we receive setup changes. I will try making that change.

brunchboy commented 7 years ago

Well, unfortunately it’s not that simple. Trying to react to both kMIDIMsgSetupChanged and kMIDIMsgPropertyChanged led to java.util.ConcurrentModificationException being thrown several times in the notifyCallback method, presumably because we are trying to handle additional MIDI changes while still processing the first. I am going to try a slightly different approach. If we get a second callback while already in the process of handling one, I am simply going to set a flag saying we need to re-run our handler when it finishes. That should let us batch multiple callbacks into a single cleanup pass at the end, and also can probably let us stop trying to discriminate on what specific kind of change has occurred, which will probably be safer anyway.

DerekCook commented 7 years ago

Good spot. Proposed change makes sense to me.

brunchboy commented 7 years ago

Unfortunately it turns out to be a deeper problem, and as it's well past my bed time already, I won't be finished tonight. And as Internet service is down in Madison as well now, this is only a quick summary from my phone. The concurrency issue is a bug resulting from client code in my application changing the listener list on another thread in response to the first environment change. I want to fix that by changing from an ArrayList to a more modern and safe ConcurrentHashmap which will also protect against double adds. That with the change mentioned above will eliminate the need to synchronize the environment change callback from CoreMidi, which will make us better citizens.

But the big problem is that the current code assumes that if the unique identifier is already known, the device is unchanged. That is clearly no longer true. I think we are going to have to make the CoreMidiDeviceInfo member no longer final, and reload it whenever the environment changes. Derek, does that sound correct and sufficient and safe to you?

So we are a bit out from a release yet. I probably won’t have time to finish this until the weekend.

DerekCook commented 7 years ago

Hi, James. I'm not clear as to why the UUID changes? I trust your judgement of course, and in my librarian use case I am not terribly interested in devices coming and going. So it is of course more important that you get it working as needed for your use case, and then mine should be fine.

If it helps, there is no reason that we (assuming it is not difficult) cannot get a release for #19 out of the way and do another for #21 when ready if splitting them suits everybody. I was just trying to save the pain of doing two releases in a short period if it was all easy :)

brunchboy commented 7 years ago

Sorry, Derek, the problem is not that the unique ID changes, but that it doesn’t change. As I feared, my phone-based brevity led to confusion. In CoreMidiDeviceProvider’s buildDeviceMap() method, we have:

// If the unique ID of the end point is not in the map then create a
// CoreMidiSource object and add it to the map
if ( !midiProperties.deviceMap.containsKey(uniqueID) ) {
        midiProperties.deviceMap.put(uniqueID,
          new CoreMidiSource(getMidiDeviceInfo(endPointReference)));
}

However, in the case where a user has edited a device name, that device’s uniqueID is already present in the device map, so we are assuming we already know everything we need to about it, and do not update the CoreMidiDeviceInfo object. So the changed name is not reflected in the values that we are reporting to our clients. Does that make more sense as an explanation?

brunchboy commented 7 years ago

So what I am saying is that I believe we need to act differently for devices that are already present in midiProperties.deviceMap: We don’t want to replace the CoreMidiSource object, since we may be tracking opened connections to it, but we do want to update it with a new CoreMidiDeviceInfo object containing up-to-date name information in case that has changed. And that is what I was asking whether it would be safe and sufficient from your perspective.

brunchboy commented 7 years ago

I would have no idea about how to tease out the changes for the two separate issue fixes into two releases, and that seems like much more work to me than simply finishing what we have started. But if you prefer to take that route, I can abandon the work I have done so far, and try again once you’ve released what you want to.

DerekCook commented 7 years ago

Hi, James

Ok I'm with you. No, do not abandon anything. It can wait as far as I'm concerned. Path of least resistance rules. 😀

DerekCook commented 7 years ago

Ps not sure if we can make CoreMidiDeviceInfo to not be final, as, from memory, MidiDevice.Info is final. But I may be barking mad and it has been a very long week!!

brunchboy commented 7 years ago

You are not mad; or at least, you are not providing evidence of that in this discussion. 😉 I have had a bit of time to investigate this, and even though Java’s MidiDevice.Info is final, our own CoreMidiSource and CoreMidiDestination classes have their own info members which we can make non-final. I hope to have an updated snapshot for the two of you to test shortly that will reflect updated device names. I am also fixing another issue I discovered: Java is constructing CoreMidiDeviceProvider over a hundred times, and each time we are adding it to the listener list to be notified of MIDI environment changes, so we are doing far more work than we need to, rebuilding the device map over and over, when the environment changes. I am going to prevent more than one instance being added to the listener list, because one update of the (shared) map will suffice.

brunchboy commented 7 years ago

OK, things are working nicely and I am in the home stretch now. It turns out that even with a simple name change, we do still receive the final kMIDIMsgSetupChanged message. So we don’t need to change which messages we react to after all, and can simply rescan the interfaces once upon receiving that. Once I get a bit more testing done, I will upload a snapshot. I hope that you both can do some testing with it, because of the variety of changes, but if it looks good we should be able to release it.