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

fix: timed out fetching a new connection from the connection pool. #4998

Merged
merged 8 commits into from
Oct 30, 2024

Conversation

youxq
Copy link
Contributor

@youxq youxq commented Sep 12, 2024

Fixes: prisma/prisma#25162

solution

Change new MakeTlsConnector, which is executed by each PostgreSql::new, to obtain it from MakeTlsConnectorManager.get_connector. The logic of get_connector is to create new MakeTlsConnector in the first call and save the created tls connector. All subsequent calls will clone the tls connector from the first call, thereby reducing the overhead of the new MakeTlsConnector.

test results

Use https://github.com/youxq/prisma-connection-test tool to test.

The bug of Timed out fetching a new connection from the connection pool. was successfully triggered before the fix.
image
It no longer appears after the fix.
image

@youxq youxq requested a review from a team as a code owner September 12, 2024 09:08
@youxq youxq requested review from Weakky and removed request for a team September 12, 2024 09:08
@CLAassistant
Copy link

CLAassistant commented Sep 12, 2024

CLA assistant check
All committers have signed the CLA.

@rwest202
Copy link

Is this ready to merge?

@aqrln aqrln self-assigned this Sep 26, 2024
@aqrln aqrln added this to the 5.21.0 milestone Sep 26, 2024
@aqrln
Copy link
Member

aqrln commented Sep 26, 2024

Thanks so much for the PR and sorry for the delay, I'll get to it soon (tomorrow or early next week).

@aqrln aqrln requested review from aqrln and removed request for Weakky September 26, 2024 10:36
Copy link

codspeed-hq bot commented Sep 26, 2024

CodSpeed Performance Report

Merging #4998 will not alter performance

Comparing youxq:fix/tls-builder (4ac07af) with main (4b67dc7)

Summary

✅ 11 untouched benchmarks

@youxq
Copy link
Contributor Author

youxq commented Sep 29, 2024

I have fixed the lint issue. Please help trigger the checks workflow again. Thanks.

@michael-rzero
Copy link

Would love to see this merged

@semoal
Copy link

semoal commented Oct 8, 2024

Seeing this error in production environment, even using pgbouncer, would be nice to ship this in the next release

@TheBubbleDuck
Copy link

Agreed with others! We are having similar issues as well and it would be great to see this make it into a release. Even if there is was an RC or beta to test it out to get more consensus and field testing if that would help community feel better about it.

@rwest202
Copy link

Looks like this has stalled out again, we are seeing this issue on our live app

@youxq
Copy link
Contributor Author

youxq commented Oct 16, 2024

@aqrln @anton-ax Hi, I have resolved the conflict. Please take a look. thanks

@tmm1
Copy link
Contributor

tmm1 commented Oct 18, 2024

@aqrln can we get some movement here

cc #5011

@michael-rzero
Copy link

Would love to see this released, would fix an issue we're seeing

@aqrln
Copy link
Member

aqrln commented Oct 23, 2024

It's on my radar, sorry for the lack of movement so far! I approved the CI run for the latest changes.

@michael-rzero
Copy link

Awesome, thank you @aqrln - looks like all passed - thank you for keeping this on the go :)

@youxq
Copy link
Contributor Author

youxq commented Oct 24, 2024

Sorry, I just fixed another rustfmt issue, please help trigger the CI again, thanks @aqrln

@ajhollowayvrm
Copy link

Would LOVE to see this merged in as I've recently run into this issue while running Prisma in Kubernetes in GCP.

@aqrln
Copy link
Member

aqrln commented Oct 29, 2024

Hey folks, I've triggered a dev release, you can test it by using Prisma version 5.22.0-integration-engines-5-22-0-22-gh-4998-integration-eb7519dcdf552de090269ebfa2beabc8e1a13e1b.1 (just make sure to install the exact version without ^, otherwise you can get other random stuff from the integration dist-tag built from other branches).

@tmm1
Copy link
Contributor

tmm1 commented Oct 29, 2024

FWIW we've been running this in production for a few days now and it works as advertised.

Copy link
Member

@aqrln aqrln left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks for doing this work and sorry for the delay in reviewing!

quaint/src/pooled.rs Outdated Show resolved Hide resolved
Co-authored-by: Alexey Orlenko <[email protected]>
@aqrln aqrln merged commit 042b086 into prisma:main Oct 30, 2024
367 of 368 checks passed
@aqrln aqrln modified the milestones: 5.21.0, 5.22.0 Oct 30, 2024
@tmm1 tmm1 mentioned this pull request Nov 6, 2024
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.

Timed out fetching a new connection from the connection pool.