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

set use_tweens to True when using the invoke_subrequest pyramid method #203

Merged
merged 6 commits into from
Jul 9, 2019

Conversation

davidcaron
Copy link
Contributor

@davidcaron davidcaron commented Jul 5, 2019

I could reproduce the connection pool issues we were having, and this should fix it. I think multiple threads were getting the same session from the factory, and the sessions were then probably not closed properly.

Also, remove pool_threadlocal which is equivalent to use_threadlocal, which is deprecated.

Explanation here: https://docs.sqlalchemy.org/en/13/orm/contextual.html#thread-local-scope

remove pool_threadlocal which is equivalent to use_threadlocal
which is deprecated
@davidcaron davidcaron requested review from dbyrns and fmigneault July 5, 2019 21:48
@codecov-io
Copy link

codecov-io commented Jul 5, 2019

Codecov Report

Merging #203 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #203      +/-   ##
==========================================
- Coverage   71.46%   71.44%   -0.03%     
==========================================
  Files          94       94              
  Lines        6848     6846       -2     
==========================================
- Hits         4894     4891       -3     
- Misses       1954     1955       +1
Impacted Files Coverage Δ
magpie/ui/utils.py 77.5% <100%> (ø) ⬆️
magpie/db.py 78.51% <100%> (-0.35%) ⬇️
magpie/api/login/login.py 47.5% <100%> (+0.62%) ⬆️
magpie/ui/login/views.py 72.54% <0%> (-3.93%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91c43d3...1148787. Read the comment docs.

Copy link
Contributor

@dbyrns dbyrns left a comment

Choose a reason for hiding this comment

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

Tested and working

Copy link
Contributor

@dbyrns dbyrns left a comment

Choose a reason for hiding this comment

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

We will postpone this PR a little bit as it cause twitcher to block all access :
2019-07-08 14:13:01,415 INFO [TWITCHER:60][waitress] Using adapter: '<class 'magpie.adapter.MagpieAdapter'>'
twitcher | 2019-07-08 14:13:01,418 ERROR [TWITCHER:33][waitress] unknown error
twitcher | Traceback (most recent call last):
twitcher | File "/opt/birdhouse/src/twitcher/twitcher/tweens.py", line 27, in ows_security_tween
twitcher | security.check_request(request)
twitcher | File "/opt/local/src/magpie/magpie/adapter/magpieowssecurity.py", line 42, in check_request
twitcher | httpError=HTTPForbidden, msgOnFail="Service query by name refused by db.")
twitcher | File "/opt/local/src/magpie/magpie/api/exception.py", line 237, in evaluate_call
twitcher | contentType=contentType)
twitcher | File "/opt/local/src/magpie/magpie/api/exception.py", line 318, in raise_http
twitcher | raise resp
twitcher | pyramid.httpexceptions.HTTPForbidden: Access was denied to this resource.

Copy link
Contributor

@dbyrns dbyrns left a comment

Choose a reason for hiding this comment

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

Need a fix for twitcher before merging

@davidcaron davidcaron changed the title Use sqlalchemy.orm.scoped_session to be thread safe set use_tweens to True when using the invoke_subrequest pyramid method Jul 8, 2019
@davidcaron
Copy link
Contributor Author

@dbyrns, @fmigneault I think I found it... when magpie used the invoke_subrequest method of pyramid, the session used within this subrequest was not properly managed by pyramid_tm because invoke_subrequest doesn't use tweens by default, and the connection was left in a transaction state.

I think using pool_threadlocal fixed it because the session obtained by the subrequest was the same as in the main request.

@davidcaron
Copy link
Contributor Author

Oh, and by default pyramid_tm uses thread locals, so as long as we let it handle the request, using multiple threads should be ok.

@dbyrns
Copy link
Contributor

dbyrns commented Jul 8, 2019

So, setting pool_threadlocal to true isn't only pushing the limit ahead, it really removes the limit and we can do as many subrequest as required without exhausting the connections, right?

@davidcaron
Copy link
Contributor Author

Yes, by setting pool_threadlocal to True, subrequests made with with invoke_subrequest will use the same database connection from the pool. I think this translates to a nested transaction. The main request has a transaction, and the subrequests each have a nested transaction within the main one.

Copy link
Collaborator

@fmigneault fmigneault left a comment

Choose a reason for hiding this comment

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

working on my side

@fmigneault fmigneault merged commit 86be087 into master Jul 9, 2019
@fmigneault fmigneault deleted the fix-scoped-sessions branch July 9, 2019 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.

4 participants