Topic/coremidi crash fix #1147

Merged
merged 3 commits into from May 23, 2015

Conversation

Projects
None yet
6 participants
@scztt
Contributor

scztt commented Jul 6, 2014

No description provided.

scztt added some commits Jul 6, 2014

MIDI: Fix a crash on mac when initializing midi after a library recom…
…pile.

On mac, sclang can crash when MIDI is initialized after a library recompile (on my machine and a few others I've used, this occurs 100% of the time). This crash occurs in prListMIDIEndpoints, due to a null devname or endname because of a previous call that errors out. Trivially, we can stop the crash by protecting against cases where MIDIObjectGetStringProperty doesn't return a proper result. These calls were ultimately failing because calling MIDIClientCreate a second time after called MIDIClientDispose results in that call failing. The documentation is unclear exactly how CoreMIDI expects these to be called, but it implies that the normal lifetime of a client is one per app launch ("It is not essential to call this function; the CoreMIDI framework will automatically dispose all MIDIClients when an application terminates.") - in any case, our use case is pretty simple, and a crash is a crash..... This fixes the core problem by only initializing MIDI once, and then keeping that MIDI client for the rest of sclang's lifetime. Effectively, gMIDIClient becomes a singleton.
@timblechmann

This comment has been minimized.

Show comment
Hide comment
@timblechmann

timblechmann Jul 7, 2014

Contributor

didn't check the code, but maybe it would make sense to provide a deinitMIDI function, which will be called at the time of the library shutdown ... i had been doing this for the new HID primitives and it should probably be done for every primitive which allocates resources/threads.

Contributor

timblechmann commented Jul 7, 2014

didn't check the code, but maybe it would make sense to provide a deinitMIDI function, which will be called at the time of the library shutdown ... i had been doing this for the new HID primitives and it should probably be done for every primitive which allocates resources/threads.

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt Jul 7, 2014

Contributor

There's already a de-initialization function, but that's precisely the problem. After initializing the midi client, then de-initializing, future attempts to initialize will fail -- causing bad bugs. We're barely using the MIDI client - it's only required as an argument to a few other calls - so there's not much massaging of code on the SC side to be done. I suspect it's either not intended to be repeatedly created and destroyed as we're doing, or if it is, nobody is doing it except for us and there are lurking bugs. Regardless, I don't see any reason why we can't just carry it over between Library rebuilds. Note that we still init and de-init midi connections we make between library recompiles - this seems to work just fine.

Contributor

scztt commented Jul 7, 2014

There's already a de-initialization function, but that's precisely the problem. After initializing the midi client, then de-initializing, future attempts to initialize will fail -- causing bad bugs. We're barely using the MIDI client - it's only required as an argument to a few other calls - so there's not much massaging of code on the SC side to be done. I suspect it's either not intended to be repeatedly created and destroyed as we're doing, or if it is, nobody is doing it except for us and there are lurking bugs. Regardless, I don't see any reason why we can't just carry it over between Library rebuilds. Note that we still init and de-init midi connections we make between library recompiles - this seems to work just fine.

@scztt scztt added this to the 3.7 milestone Apr 14, 2015

@scztt scztt assigned crucialfelix and unassigned crucialfelix Apr 19, 2015

@scztt scztt referenced this pull request Apr 19, 2015

Closed

MIDI listEndPoints crash #353

@crucialfelix

This comment has been minimized.

Show comment
Hide comment
@crucialfelix

crucialfelix May 12, 2015

Member

What's the best way to test this ? create a midi responder and recompile the library, repeat vigorously ?

Member

crucialfelix commented May 12, 2015

What's the best way to test this ? create a midi responder and recompile the library, repeat vigorously ?

@crucialfelix

This comment has been minimized.

Show comment
Hide comment
@crucialfelix

crucialfelix May 13, 2015

Member

arg. this is so old that it won't build with my current up to date qt5 sc set up.

Member

crucialfelix commented May 13, 2015

arg. this is so old that it won't build with my current up to date qt5 sc set up.

@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon May 22, 2015

Member

Regardless, I don't see any reason why we can't just carry it over between Library rebuilds.

A library rebuild is supposed to behave exactly like a restart of sclang, so maybe better not.

Member

telephon commented May 22, 2015

Regardless, I don't see any reason why we can't just carry it over between Library rebuilds.

A library rebuild is supposed to behave exactly like a restart of sclang, so maybe better not.

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt May 23, 2015

Contributor

@telephon - You're right - but, this is seemingly a problem with the CoreMIDI API's, so there's no other option to avoid the crash. AFAICT once we've called MIDIClientDispose, we can't call MIDIClientCreate again. In lieu of a specific problem happening because of the client re-use, it seems better to opt for avoiding the crash....

Contributor

scztt commented May 23, 2015

@telephon - You're right - but, this is seemingly a problem with the CoreMIDI API's, so there's no other option to avoid the crash. AFAICT once we've called MIDIClientDispose, we can't call MIDIClientCreate again. In lieu of a specific problem happening because of the client re-use, it seems better to opt for avoiding the crash....

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt May 23, 2015

Contributor

@crucialfelix - I saw this all the time in a workflow that looked like: (a) compile lang (b) create and init client (c) use midi, (d) recompile (e) repeat..... In an afternoon of working, I would see this crash ten or twelve times - not on every recompile, but it was not uncommon either.

Contributor

scztt commented May 23, 2015

@crucialfelix - I saw this all the time in a workflow that looked like: (a) compile lang (b) create and init client (c) use midi, (d) recompile (e) repeat..... In an afternoon of working, I would see this crash ten or twelve times - not on every recompile, but it was not uncommon either.

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt May 23, 2015

Contributor

@crucialfelix - I just updated this branch but didn't see any merge conflicts or anything - what's the build problem you're seeing?

Contributor

scztt commented May 23, 2015

@crucialfelix - I just updated this branch but didn't see any merge conflicts or anything - what's the build problem you're seeing?

@crucialfelix

This comment has been minimized.

Show comment
Hide comment
@crucialfelix

crucialfelix May 23, 2015

Member

I've experienced the bug myself in the past. Very happy to see a potential
fix.

If I check out the branch I couldn't just build it because I have to switch
my qt, clean build, set cmake the same way we used to do it. The new build
system is much nicer

Or we just merge it and test in master.
On Sat, May 23, 2015 at 8:38 PM scztt notifications@github.com wrote:

@crucialfelix https://github.com/crucialfelix - I just updated this
branch but didn't see any merge conflicts or anything - what's the build
problem you're seeing?


Reply to this email directly or view it on GitHub
#1147 (comment)
.

Member

crucialfelix commented May 23, 2015

I've experienced the bug myself in the past. Very happy to see a potential
fix.

If I check out the branch I couldn't just build it because I have to switch
my qt, clean build, set cmake the same way we used to do it. The new build
system is much nicer

Or we just merge it and test in master.
On Sat, May 23, 2015 at 8:38 PM scztt notifications@github.com wrote:

@crucialfelix https://github.com/crucialfelix - I just updated this
branch but didn't see any merge conflicts or anything - what's the build
problem you're seeing?


Reply to this email directly or view it on GitHub
#1147 (comment)
.

@scztt

This comment has been minimized.

Show comment
Hide comment
@scztt

scztt May 23, 2015

Contributor

I'm okay with merging and testing in master - I would consider this a safe fix. @telephon - can you merge if you're okay, or else let us know if there are any problems related to reusing the MIDI client?

Contributor

scztt commented May 23, 2015

I'm okay with merging and testing in master - I would consider this a safe fix. @telephon - can you merge if you're okay, or else let us know if there are any problems related to reusing the MIDI client?

telephon added a commit that referenced this pull request May 23, 2015

@telephon telephon merged commit af7d730 into supercollider:master May 23, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@telephon

This comment has been minimized.

Show comment
Hide comment
@telephon

telephon May 23, 2015

Member

I don't use MIDI, so I can't test. But I think this fix for now is better than nothing.

Member

telephon commented May 23, 2015

I don't use MIDI, so I can't test. But I think this fix for now is better than nothing.

@redFrik

This comment has been minimized.

Show comment
Hide comment
@redFrik

redFrik May 24, 2015

Contributor

@scztt many thanks for fixing this longstanding issue.

i can confirm that it is now fixed. in my previous build (21c661b) and earlier it was easy to crash sc with...

MIDIClient.init
//recompile
MIDIClient.init
//crash -> 'Interpreter has crashed or stopped forcefully. [Exit code: 0]'

and you'd get this even without any midi gear connected.
with latest build it doesn't crash. thanks again.

Contributor

redFrik commented May 24, 2015

@scztt many thanks for fixing this longstanding issue.

i can confirm that it is now fixed. in my previous build (21c661b) and earlier it was easy to crash sc with...

MIDIClient.init
//recompile
MIDIClient.init
//crash -> 'Interpreter has crashed or stopped forcefully. [Exit code: 0]'

and you'd get this even without any midi gear connected.
with latest build it doesn't crash. thanks again.

@crucialfelix

This comment has been minimized.

Show comment
Hide comment
@crucialfelix

crucialfelix May 24, 2015

Member

All Hail @scztt Exterminator of The Bug !

Member

crucialfelix commented May 24, 2015

All Hail @scztt Exterminator of The Bug !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment