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 TLS certificate files when they change #4290

Closed
wants to merge 4 commits into from
Closed

Conversation

fmoor
Copy link
Member

@fmoor fmoor commented Aug 23, 2022

fixes #4277

edb/server/main.py Outdated Show resolved Hide resolved
edb/server/main.py Outdated Show resolved Hide resolved
@elprans
Copy link
Member

elprans commented Aug 23, 2022

Is there an asyncio-compatible watching library we can use instead of spawning threads?

@1st1
Copy link
Member

1st1 commented Aug 23, 2022

Is there an asyncio-compatible watching library we can use instead of spawning threads?

No popular and trusted one afaik. Maybe we spawn a thread and just check mtime in a loop every 5 seconds?

@elprans
Copy link
Member

elprans commented Aug 23, 2022

Is there an asyncio-compatible watching library we can use instead of spawning threads?

No popular and trusted one afaik. Maybe we spawn a thread and just check mtime in a loop every 5 seconds?

Threads mean GIL. If it can't be in the event loop, then a monitoring subprocess sending SIGHUP to the main process seems better to me.

@1st1
Copy link
Member

1st1 commented Aug 23, 2022

Threads mean GIL. If it can't be in the event loop, then a monitoring subprocess sending SIGHUP to the main process seems better to me.

That's too complicated.

@fantix how about we add a hidden API to uvloop to expose libuv watchers? That should be easy.

@fmoor
Copy link
Member Author

fmoor commented Aug 23, 2022

There is https://github.com/samuelcolvin/watchfiles which is mostly an interface to https://github.com/notify-rs/notify but its not 1.0 yet.

@1st1
Copy link
Member

1st1 commented Aug 23, 2022

There is https://github.com/samuelcolvin/watchfiles which is mostly an interface to https://github.com/notify-rs/notify but its not 1.0 yet.

It also uses threads for its async API.

@fantix
Copy link
Member

fantix commented Aug 28, 2022

@fantix how about we add a hidden API to uvloop to expose libuv watchers? That should be easy.

I'm looking into this now - we can start with tweaking MagicStack/uvloop#474, the basic API should be pretty straightforward.

Even though we don't need the recursive watching in this PR, it'll be needed by the edgedb-python codegen --watch mode. However, libuv doesn't support recursive watching out of the box on Linux, and libuv is likely NOT going to add support for that. So IMO we should at least make the hidden uvloop API friendly-enough for implementing recursive watching, given aside of whether we want to support Linux recursive watching directly in uvloop. I'll try something on top of @jensbjorgensen's implementation, and start a new thread in the uvloop repo for further discussion.

Oh, edgedb-python doesn't depend on uvloop, and --watch mode will probably be just an extra feature that doesn't necessarily require uvloop for watching changes recursively. So it'll be easy then, literally quoting Yury's "we add a hidden API to uvloop to (simply) expose libuv watchers".

@1st1
Copy link
Member

1st1 commented Sep 19, 2022

@fantix can you pick this one up given that we have the necessary new API in uvloop?

edb/server/server.py Outdated Show resolved Hide resolved
edb/server/server.py Outdated Show resolved Hide resolved
edb/server/server.py Show resolved Hide resolved
@elprans
Copy link
Member

elprans commented Oct 10, 2022

Closing in favor of the merged #4297.

@elprans elprans closed this Oct 10, 2022
@elprans elprans deleted the reload-tls branch October 10, 2022 20:27
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.

Reload TLS cerificate files when they change
4 participants