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

Change the shutdown procedure to deregister fabio from the registry and then shutdown the proxy #908

Conversation

martinivanov
Copy link
Contributor

The current shutdown procedure shuts down all proxies and then de-registers Fabio from the service registry. This may cause some requests to fail while a Fabio instance is shutting down. This PR reverses the order and introduces a de-register grace period, causing Fabio to wait for a configurable period after de-registering from a registry and then shutting down the proxies. This way no new requests will be sent to the stopping instances, while giving time to running requests to finish.

@CLAassistant
Copy link

CLAassistant commented Nov 3, 2022

CLA assistant check
All committers have signed the CLA.

@martinivanov martinivanov changed the title Change the shutdown procedure to deregister fabio from all registries and then shutdown the proxy Change the shutdown procedure to deregister fabio from the registry and then shutdown the proxy Nov 3, 2022
@nathanejohnson
Copy link
Member

I like the idea of changing the shutdown procedure order. As for the configurable grace period, there is already a proxy.shutdownwait .

https://fabiolb.net/ref/proxy.shutdownwait/

Would that solve your issue?

@martinivanov
Copy link
Contributor Author

martinivanov commented Nov 4, 2022

It doesn't serve the same purpose. The shutdownwait gives time to currently in-flight requests/connections finish, while new ones are declined. In the best case proxy.shutdownwait and the grace period will work together for pretty much 0 dropped requests. The main point is that some requests may still reach Fabio after it de-registers from Consul for example and by not waiting for some time they would be declined due to the proxy being shut down.

@nathanejohnson
Copy link
Member

So after digging through the code, it looks like tcp, grpc, and https+tcp+sni proxies will all wait until the context expires before returning. However, the http proxy does not, it will wait at most up to the context deadline but if all listeners finish with connections before that, it returns early. So as much as I hate the inconsistency here in fabio, I don't think it's worth it to change existing behavior.

So if you could add some documentation to

  1. the fabio.properties file in the comments, following the other examples in there
  2. add a markdown document with similar documentation inside of docs/content/ref

I will get this merged. Thank you!

@martinivanov
Copy link
Contributor Author

Done!

@nathanejohnson nathanejohnson merged commit 1693793 into fabiolb:master Nov 4, 2022
@nathanejohnson
Copy link
Member

Thank you for your contribution!

@martinivanov martinivanov deleted the feat/registry-deregister-grace-period branch November 4, 2022 14:19
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.

3 participants