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

Reverse shutdown order of LifecycleHandlers #2583

Merged
merged 2 commits into from
Mar 23, 2021
Merged

Reverse shutdown order of LifecycleHandlers #2583

merged 2 commits into from
Mar 23, 2021

Conversation

AZm87
Copy link
Contributor

@AZm87 AZm87 commented Mar 16, 2021

Reverse the shutdown order of LifecycleHandlers so that handlers that depend on previous handlers are shutdown in the correct order. (#2583)

@Joannis
Copy link
Member

Joannis commented Mar 22, 2021

This PR seems really sane to me. I'm worried if there's an impact on existing applications though. I bet @gwynne has a good idea about that, possibly a good solution. We could add a boolean to solve the migration path, with the default value being how it worked before.

@Joannis Joannis requested a review from gwynne March 22, 2021 08:59
@gwynne
Copy link
Member

gwynne commented Mar 22, 2021

I don't think there's too much danger for existing applications; it's effectively useless to depend on the original shutdown order, so making it more explicitly deterministic in a useful way shouldn't be breaking unless someone was already doing something they shouldn't.

@0xTim 0xTim added the semver-patch Internal changes only label Mar 23, 2021
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

This looks good and makes sense to me. I'll merge this once the release bot is back up and running

@0xTim 0xTim changed the title reversed shutdown order of LifecycleHandlers Reverse shutdown order of LifecycleHandlers Mar 23, 2021
@0xTim
Copy link
Member

0xTim commented Mar 23, 2021

Original comment:

I want my vapor backend to count the requests to allow each user only a certain number of requests.
Since there are a lot of requests, I don't want to query the database for each request just to check and decrease a counter.
Therefore I want to download the data from the database once at the beginning and then cache them locally.
This cache is then regularly synchronized with the database.
To write the data on shutdown into the database I implemented a LifecycleHandler.
The problem is, that the LifecycleHandler has to be registered after the database, because it needs the data from the database.
Because of this the LifecycleHandler's shutdown function is called when the database connection is already closed.
This leads to the fact that a new connection is created to synchronize the cache.
This connection will not be closed afterwards, because the LifecycleHandler of the database already finished.
This leads to the error "ConnectionPool.shutdown() was not called before deinit."
To solve this I reversed the order of the LifecycleHandlers for the shutdown.

@0xTim 0xTim merged commit d9c7092 into vapor:main Mar 23, 2021
@VaporBot
Copy link
Contributor

These changes are now available in 4.41.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants