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

copy_current_request_context copies old session object #2935

Closed
dchevell opened this issue Oct 5, 2018 · 1 comment

Comments

@dchevell
Copy link
Contributor

commented Oct 5, 2018

Expected Behavior

When using copy_current_request_context in the middle of a request, I'm expecting it to copy the current state of flask.request and flask.session at the time it's called.

Actual Behavior

The current state of flask.request is copied, but flask.session is from before the current request and doesn't contain changes made during the current request, before copy_current_request_context was called. To me this seems like unexpected behaviour. If it's not I'd like to understand a bit more about why, and also to find out whether there's any workaround or alternative approach I can take.

Example

Here's a fully functional example you can save and run. It adds a test value to flask.request and flask.session during the request, then uses copy_current_request_context to decorate a method that will retrieve those same values in another thread. Two requests are made; on the first request the copied context can find the new value added to flask.request but cannot find the value added to flask.session. On the second request, the copied context can find the value added to flask.request, and now finds the value that was previously added to flask.session in the first request.

import concurrent.futures
import random

from flask import Flask, copy_current_request_context, request, session


app = Flask(__name__)
app.config['SECRET_KEY'] = 'test'
executor = concurrent.futures.ThreadPoolExecutor()


@app.route('/')
def session_context():
    test_value = random.randint(1, 1001)
    request.test_value = test_value
    session['TEST_VALUE'] = test_value

    original_context = (
        ('request', request.test_value),
        ('session', session.get('TEST_VALUE'))
    )

    @copy_current_request_context
    def debug_session():
        return (
            ('request', request.test_value),
            ('session', session.get('TEST_VALUE'))
        )

    future = executor.submit(debug_session)
    print('original_context:', original_context)
    print('copied_context:', future.result())
    return 'ok'


if __name__ == '__main__':
    client = app.test_client()
    print('### First request ###')
    client.get('/')
    print('### Second request ###')
    client.get('/')

Example output:

$ python session_context.py
### First request ###
original_context: (('request', 27), ('session', 27))
copied_context: (('request', 27), ('session', None))
### Second request ###
original_context: (('request', 63), ('session', 63))
copied_context: (('request', 63), ('session', 27))

Environment

  • Python version: 3.7.0
  • Flask version: Flask==1.0.2
  • Werkzeug version: Werkzeug==0.14.1

Also, for whatever it's worth: I'm investigating this as part of improving this project: https://github.com/dchevell/flask-executor

@dchevell

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2018

I've found a solution for this - but I don't trust it because I don't have a strong enough knowledge of the Werkzeug internals to know if there are unintended side effects.

After making a copy of the request context, you can simply give it a copy of the current session object. Here's a modified version of copy_current_request_context that does just that:

def copy_current_request_context(f):
    top = _request_ctx_stack.top
    if top is None:
        raise RuntimeError('This decorator can only be used at local scopes '
            'when a request context is on the stack.  For instance within '
            'view functions.')
    reqctx = top.copy()
    reqctx.session = session.copy()
    def wrapper(*args, **kwargs):
        with reqctx:
            return f(*args, **kwargs)
    return update_wrapper(wrapper, f)

reqctx.session = session.copy() is the addition here.

With this change, my example produces the expected behaviour. The Flask test suite passes, so there's no obvious side effects I can see.

I would love to have the built in copy_current_request_context produce this behaviour, but I suppose the question becomes: should it? I'm guessing the behaviour I've run into exists because is not because copy_current_request_context is copying an old session object as I originally thought, but instead simply isn't copying anything. It stands to reason that as a byproduct of that flask.session proxy points to the previous state of the session object in the copied request context. Since the end result is that the request and session state are inconsistent I think my solution still makes sense, so I've opened #2936 to propose the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.