Skip to content
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

Reload option doesnt work with unix sockets #722

Closed
euri10 opened this issue Jul 24, 2020 · 3 comments
Closed

Reload option doesnt work with unix sockets #722

euri10 opened this issue Jul 24, 2020 · 3 comments

Comments

@euri10
Copy link
Member

euri10 commented Jul 24, 2020

Was about to report the same thing, but with unix sockets.

Originally posted by @magnusjjj in #553 (comment)

@magnusjjj
Copy link

Yeah, uh, I have looked through the source.

main.py:346

    if config.should_reload:
        sock = config.bind_socket()
        supervisor = ChangeReload(config, target=server.run, sockets=[sock])
        supervisor.run()
    elif config.workers > 1:
        sock = config.bind_socket()
        supervisor = Multiprocess(config, target=server.run, sockets=[sock])
        supervisor.run()
    else:
        server.run()

That. It seems like there are three kinds of 'processes', and they can't "stack".

The problem lies in that unix sockets, and passing a slew of premade sockets, require different calls to setup loops with asyncio, if I read the code correctly, as seen in main.py:445

So it's the same bug for both having workers and the reloader.
Creation of the sockets should probably be moved to the config.bind_socket (or rather, bind_sockets i guess), and then later code figuring out whether or not the sockets given to them are unix sockets or workers. This is my first glance at the code though.

@euri10
Copy link
Member Author

euri10 commented Sep 29, 2020

yes you're right I think this would need a refactor, a good example would be in hypercorn I think.
lacking time to tackle on this though

@Kludex
Copy link
Sponsor Member

Kludex commented Jun 18, 2022

It looks like this was fixed by #1009.

@Kludex Kludex closed this as completed Jun 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants