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

Fix app.makeSingleInstance hanging on posix systems #10518

Merged
merged 7 commits into from Sep 20, 2017

Conversation

Projects
None yet
2 participants
@MarshallOfSound
Member

MarshallOfSound commented Sep 14, 2017

Wait for the IO thread to be a thing before attempting to listen on the socket

Fixes #9880

@MarshallOfSound MarshallOfSound requested a review from zcbenz Sep 14, 2017

@@ -183,6 +184,8 @@ void AtomBrowserMainParts::PreMainMessageLoopRun() {
std::unique_ptr<base::DictionaryValue> empty_info(new base::DictionaryValue);
Browser::Get()->DidFinishLaunching(*empty_info);
#endif
atom::api::App::Get()->PreMainMessageLoopRun();

This comment has been minimized.

@zcbenz

zcbenz Sep 15, 2017

Contributor

For consistent behavior on different platforms, this should be put before the WillFinishLaunching call, so the actual listening starts before the ready event.

This comment has been minimized.

@zcbenz

zcbenz Sep 15, 2017

Contributor

For clean architecture the classes under api namespace usually should not be accessed by the outside namespace, otherwise the browser initialization code would have to know when the module system starts.

A cleaner design to achieve this is to add PreMainMessageLoopRun to BrowserObserver and then call Browser::Get()->PreMainMessageLoopRun().

sock));
sock_ = sock;
if (atom::Browser::Get()->is_ready()) {

This comment has been minimized.

@zcbenz

zcbenz Sep 15, 2017

Contributor

You can use BrowserThread::IsMessageLoopValid(BrowserThread::IO) to avoiding involving atom::Browser here.

I'm not sure why DCHEK does not work in this file, but you should remove the previous call since it will abort after we fixed the DCHECK.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Sep 15, 2017

If I never have to debug this threading single instance locking stuff every again I'll be a happy person 😆

@zcbenz

zcbenz approved these changes Sep 20, 2017

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Sep 20, 2017

@MarshallOfSound I pushed a fix for the singleton callback being called on a non-UI thread, which should be able to fix the root cause.

MarshallOfSound and others added some commits Sep 14, 2017

Fix app.makeSingleInstance hanging on posix systems
Wait for the IO thread to be a thing before attempting to listen on the socket

Fixes #9880
Refactor as per @zcbenz comments
Also fix issue where we run the single instance callback *not* on the UI thread,
this apparently results in a hung process.

@MarshallOfSound MarshallOfSound merged commit c4cfb3e into master Sep 20, 2017

7 of 8 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
electron-mas-x64 Build #5135 succeeded in 12 min
Details
electron-osx-x64 Build #5119 succeeded in 13 min
Details

@zcbenz zcbenz deleted the fix-makesingleinstance branch Sep 20, 2017

@MarshallOfSound MarshallOfSound referenced this pull request Sep 23, 2017

Merged

Backporting changes for 1.7.8 #10586

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