Skip to content

don't launch if server fails; kill server on exit #537#541

Merged
snoyberg merged 2 commits intoyesodweb:masterfrom
simonmichael:master
May 2, 2016
Merged

don't launch if server fails; kill server on exit #537#541
snoyberg merged 2 commits intoyesodweb:masterfrom
simonmichael:master

Conversation

@simonmichael
Copy link
Copy Markdown
Contributor

Make the server thread and launch/monitor thread more aware of each
other's failures, using async. Now if the server fails to start we won't
launch a browser. Also when the main thread terminates (eg from ctrl-c
in GHCI), we make sure the server thread does too. There is now a 0.1s
delay before launching the browser, which also helps ensure the server
is ready to serve.

@snoyberg
Copy link
Copy Markdown
Member

I don't like relying on timing assumptions to make this work. Warp has a setting to run some code after initialization but before the main loop, which would allow handling this more robustly

@simonmichael
Copy link
Copy Markdown
Contributor Author

Great! PR updated.


-- launch the browser, after waiting until the server
-- signals that it's ready (or raises a startup exception)
let wait = do
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use an IORef here? If you use an MVar, then you can get blocking behavior and not need to worry about polling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfamiliarity/doubt about exception handling. It seems to work, PR updated...

Make the server thread and launch/monitor thread more aware of each
other, using async. Now, we won't launch a browser until the server is
ready to serve pages, and we won't launch it at all if the server fails
to start. Also we make sure to terminate the server thread whenever the
main thread terminates, eg from ctrl-c in GHCI.
@simonmichael
Copy link
Copy Markdown
Contributor Author

Nice! I appreciate the review.

@snoyberg
Copy link
Copy Markdown
Member

snoyberg commented May 2, 2016

Alright, I'm out of ideas :). I'll wait for Travis to check before merging, but LGTM.

@snoyberg snoyberg merged commit 2a755b7 into yesodweb:master May 2, 2016
snoyberg added a commit that referenced this pull request May 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants