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

[Don't merge] Use private container to get database connection #1766

Merged

Conversation

t-ae
Copy link
Contributor

@t-ae t-ae commented Jul 21, 2018

Multiple requests can grab same database connection.
See DatabaseKit issue #43 for details.

Request tries to get database connection from self (as Container). Internally the connection is cached in the container.

But Request is AliasedContainer, so cache is shared among the requests.
If the server receives two (or more) requests simultaneously, they use the same database connection and it can crash here for MySQL, or somewhere else for other DB.

To make cache not shared, I changed to use privateContainer instead of Request itself.
There is similar situation so I think it may work well.

Checklist

  • Circle CI is passing (code compiles and passes tests).
  • There are no breaking changes to public API.
  • New test cases have been added where appropriate.
  • All new code has been commented with doc blocks ///.

@t-ae t-ae changed the title Use private container to get database connection [Don't merge] Use private container to get database connection Jul 23, 2018
@t-ae
Copy link
Contributor Author

t-ae commented Jul 23, 2018

Well, it turned out my implementation has a problem.

To make cache not shared, I changed to use privateContainer instead of Request itself to call requestCachedConnection.
But internally, requestCachedConnection tries to get DatabaseConnectionPool. The connection pools are also separated if we use privateContainer.

@t-ae
Copy link
Contributor Author

t-ae commented Jul 23, 2018

Created another PR for DatabaseKit.
vapor/database-kit#44

After that is merged, I'll continue this PR.

@tanner0101 tanner0101 added the bug Something isn't working label Jul 25, 2018
@tanner0101 tanner0101 self-assigned this Jul 25, 2018
@tanner0101
Copy link
Member

@t-ae thanks for the great work tracking down this bug. I've merged in your PR to DatabaseKit and tagged as 1.3.0. You should be able to continue on this PR now 👍

@tanner0101 tanner0101 added this to the 3.0.7 milestone Jul 25, 2018
Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

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

I went ahead and made the update to DBKit 1.3.0. Thanks again for this PR!

@tanner0101 tanner0101 merged commit e8bc824 into vapor:master Jul 25, 2018
@penny-coin
Copy link

Hey @t-ae, you just merged a pull request, have a coin!

You now have 3 coins.

@pankajsoni19
Copy link

Package.json still uses db kit 1.0.0. Can we update the supporting dependencies for vapor?

@t-ae t-ae deleted the fix-two-requests-can-get-same-db-connection branch March 18, 2021 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Vapor 3
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

4 participants