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

Reduces pool resource locking #496

Closed
wants to merge 2 commits into from
Closed

Reduces pool resource locking #496

wants to merge 2 commits into from

Conversation

diogob
Copy link
Contributor

@diogob diogob commented Feb 17, 2016

This PR was based on a conversation I had with @ruslantalpa in the gitter and on some ad-hoc tests using threadDelay.

I'm still not sure if I should include some kind of test or benchmark to validate this change.

The change is very simple though, I return the results from withResource function before applying the respond continuation. This ensures that the pool resource is freed as soon as the database operation is complete

…pond continuation. This ensures that the pool resource is freed as soon as the database operation is complete
@begriffs
Copy link
Member

Makes sense, thanks for the fix.

I bet we could create a regression test case. Could fork a bunch of threads in test that all call a stored procedure which raises an exception. The test would check that it never gets that "current transaction is aborted" error.

Right c -> do
resOrError <- handleReq c
either (respond . pgErrResponse) respond resOrError
either (return . pgErrResponse) return resOrError
Copy link
Member

Choose a reason for hiding this comment

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

See if this simplification compiles

return $ either pgErrResponse id resOrError

@begriffs
Copy link
Member

Tonight I'm looking at how to apply the suggestion from hspec/hspec-wai#32 to make parallel requests in a test case.

@diogob
Copy link
Contributor Author

diogob commented Feb 18, 2016

Cool, because I would get stuck trying to do that 😜

@begriffs
Copy link
Member

Closing this PR and moving over to the other one with a test added.

@begriffs begriffs closed this Feb 21, 2016
@diogob
Copy link
Contributor Author

diogob commented Feb 21, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants