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

Stop some autosave crashes #3841

Merged
merged 2 commits into from Oct 1, 2017

Conversation

Projects
3 participants
@DomClark
Member

DomClark commented Sep 29, 2017

Move autosaving back to the main thread as Song::saveProjectFile doesn't appear to be thread-safe. Instead prevent autosave triggering during main thread VST communication by tracking call depth to RemotePlugin::waitForMessage.

Prevent autosave triggering during project load.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Sep 29, 2017

Member

Move autosaving back to the main thread as Song::saveProjectFile doesn't appear to be thread-safe.

Right. Until recently auto-saving was not active while playing for some reason. I guess not being thread safe was the reason. We added the option to turn this on and I've been monitoring the save files for issues but haven't found any. Would this PR address this issue too?

Member

zonkmachine commented Sep 29, 2017

Move autosaving back to the main thread as Song::saveProjectFile doesn't appear to be thread-safe.

Right. Until recently auto-saving was not active while playing for some reason. I guess not being thread safe was the reason. We added the option to turn this on and I've been monitoring the save files for issues but haven't found any. Would this PR address this issue too?

@DomClark

This comment has been minimized.

Show comment
Hide comment
@DomClark

DomClark Sep 29, 2017

Member

Would this PR address this issue too?

I shouldn't think so, unfortunately; autosaving always used to be in the main thread, I moved it out in #3776 as some other changes there would cause it to run during VST communication and hang trying to acquire a mutex twice in the same thread. That particular change would cause occasional crashes due to some concurrent modification to the song, so this PR is primarily a new, safer attempt at not autosaving during VST communication.

I'm not sure why autosave isn't always active during playback, but given that there's an option to enable it, it seems less likely that it's because it might crash and more likely that it's because it's CPU intensive.

Member

DomClark commented Sep 29, 2017

Would this PR address this issue too?

I shouldn't think so, unfortunately; autosaving always used to be in the main thread, I moved it out in #3776 as some other changes there would cause it to run during VST communication and hang trying to acquire a mutex twice in the same thread. That particular change would cause occasional crashes due to some concurrent modification to the song, so this PR is primarily a new, safer attempt at not autosaving during VST communication.

I'm not sure why autosave isn't always active during playback, but given that there's an option to enable it, it seems less likely that it's because it might crash and more likely that it's because it's CPU intensive.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Sep 29, 2017

Member

... more likely that it's because it's CPU intensive.

Yes. We/I weren't completely sure. It didn't work on XP for instance because it would overload the system.

Member

zonkmachine commented Sep 29, 2017

... more likely that it's because it's CPU intensive.

Yes. We/I weren't completely sure. It didn't work on XP for instance because it would overload the system.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Sep 30, 2017

Member

Tested. Unfortunately I can't replicate the issue so my test is of little value. I've tried and import large files with vst tracks but I can't make it crash and auto-save will not trigger at all for me, with or without this fix until the project has finished loading.

Member

zonkmachine commented Sep 30, 2017

Tested. Unfortunately I can't replicate the issue so my test is of little value. I've tried and import large files with vst tracks but I can't make it crash and auto-save will not trigger at all for me, with or without this fix until the project has finished loading.

@PhysSong PhysSong added this to In Progress in Release 1.2.0 RC4 Oct 1, 2017

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Oct 1, 2017

Member

@DomClark @PhysSong Since I don't see any new issues from this, maybe we should merge it and leave the testing to the user?

Member

zonkmachine commented Oct 1, 2017

@DomClark @PhysSong Since I don't see any new issues from this, maybe we should merge it and leave the testing to the user?

@Umcaruje Umcaruje merged commit 9407e84 into LMMS:stable-1.2 Oct 1, 2017

1 check passed

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

@Umcaruje Umcaruje moved this from In Progress to Done in Release 1.2.0 RC4 Oct 1, 2017

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