Skip to content

Conversation

@rghosh0
Copy link
Contributor

@rghosh0 rghosh0 commented Sep 17, 2024

Fixes #22578 ("Windows: emrun.py does not exit as expected")

We set the server's socket itself to non-blocking before shutdown. This way the threads can still be joined and the ordinary and cleaner sys.exit(...) exit function can be used. Here is a diff that we propose to be adopted (which makes this behaviour a program option for emrun).

emrun.py Outdated
page_exit_code = int(data[6:])
logv('Web page has quit with a call to exit() with return code ' + str(page_exit_code) + '. Shutting down web server. Pass --serve-after-exit to keep serving even after the page terminates with exit().')
if emrun_options.force_exit:
self.server.socket.setblocking(False)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a downside to always doing this? I mean, instead of adding an option, to always set nonblocking after the page exits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From our side there is no downside specific to this change. We just thought that it might be categorically better received if it were presented as an option, but there is no downside to always doing this before shutdown.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then I think it's better to avoid adding an option actually. That way it will be fully tested.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good.

@sbc100 do you have any concerns here?

@sbc100
Copy link
Collaborator

sbc100 commented Sep 25, 2024

I wish I understood better the naunace of what is happening.. but I fear that might take an inordinate amount of time. No concerns if you are ok with it @kripken

@kripken
Copy link
Member

kripken commented Sep 25, 2024

Yeah, I don't fully understand the deadlock either, but this seems harmless to do during shutdown anyhow, so since it fixes issues in practice on windows, let's land it.

Thanks @rghosh0 !

@kripken kripken merged commit 17463f7 into emscripten-core:main Sep 25, 2024
25 of 28 checks passed
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.

Windows: emrun.py does not exit as expected

3 participants