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 #2935: Copy current session object in copy_current_request_context #2936

Merged
merged 5 commits into from Nov 4, 2018

Conversation

Projects
None yet
4 participants
@dchevell
Contributor

dchevell commented Oct 9, 2018

Updates flask.copy_current_request_context to add a copy of the current session object to the request context copy. This prevents flask.session pointing to an out-of-date object.

  • add tests that fail without the patch
  • ensure all tests pass with pytest
  • add documentation to the relevant docstrings or pages
  • add versionadded or versionchanged directives to relevant docstrings
  • add a changelog entry if this patch changes code

Please refer to #2935 for background on this

@ThiefMaster

This comment has been minimized.

Member

ThiefMaster commented Oct 9, 2018

Looks good, but please shorten your commit messages a bit. General recommendation is max 51 chars in the first line, and yours are even beyond the limit where GitHub truncates them.

Also, I wonder if we should do a deep copy on the session... otherwise people might get a bad surprise in case of mutable objects stored there.

dchevell added some commits Oct 6, 2018

@dchevell

This comment has been minimized.

Contributor

dchevell commented Oct 9, 2018

Fixed the commit messages @ThiefMaster.

Re: deep copying, you're right - though it's worth noting that the same issue is already true for flask.request which is also susceptible to shallow copy issues with mutable objects. All the same I reproduced a shallowcopy issue on flask.session with threading (can't replicate it with greenlet) and confirmed that it's easily fixed by using copy.deepcopy(session) so I'm happy to make that change

@dchevell

This comment has been minimized.

Contributor

dchevell commented Oct 9, 2018

@ThiefMaster is there a chance users would store things in the session object that would fail a deepcopy? I don't want to break people's code. Perhaps a more conservative approach would be:

try:
    reqctx.session = deepcopy(session)
except TypeError:
    reqctx.session = session.copy()
@ThiefMaster

This comment has been minimized.

Member

ThiefMaster commented Oct 9, 2018

https://github.com/pallets/flask/blob/master/flask/json/tag.py

By default only the object types handled in there can be stored in a session

@dchevell

This comment has been minimized.

Contributor

dchevell commented Oct 9, 2018

Ah, no worries then. Alright, switched to deepcopy.

@dchevell

This comment has been minimized.

Contributor

dchevell commented Oct 13, 2018

Anything further needed to review this @ThiefMaster ?

reqctx = flask._request_ctx_stack.top.copy()
reqctx.session = deepcopy(flask.session)

This comment has been minimized.

@ThiefMaster

ThiefMaster Oct 13, 2018

Member

i don't think this should be in the test.

This comment has been minimized.

@dchevell

dchevell Oct 13, 2018

Contributor

Isn't the purpose of test_greenlet_context_copying to test the raw context copying functionality, separately from test_greenlet_context_copying_api? Without that line, the test fails. I can remove the session test from that altogether, but in that case I'm not sure what test_greenlet_context_copying is testing for.

This comment has been minimized.

@ThiefMaster

ThiefMaster Oct 13, 2018

Member

ah I didn't realize this is some greenlet stuff. nevermind then i guess.

but in that case we're missing a testcase for your feature in a normal wsgi context

This comment has been minimized.

@dchevell

dchevell Oct 13, 2018

Contributor

Hmm, in that case shouldn't there already be a test for copy_current_request_context outside of the greenlet testing? This is just a small modification to that existing feature which seems to be created / intended only for use within greenlets or threads.

This comment has been minimized.

@ThiefMaster

ThiefMaster Oct 13, 2018

Member

yes... probably. i hoped we'd have such a test. :/

This comment has been minimized.

@dchevell

dchevell Oct 13, 2018

Contributor

I can write one but it seems a bit outside the scope of this change in that case - I'm not actually certain what that test should look like, because the feature doesn't really do anything useful when used directly inline without a greenlet/thread. I can write one anyway but that might turn this PR into something else :)

This comment has been minimized.

@ThiefMaster

ThiefMaster Oct 13, 2018

Member

agreed, let's keep this pr as it is

This comment has been minimized.

@dchevell

dchevell Oct 13, 2018

Contributor

Cool, thank you @ThiefMaster ! Let me know if anything else needs another look.

@ThiefMaster ThiefMaster requested a review from davidism Oct 13, 2018

@davidism

This comment has been minimized.

Member

davidism commented Oct 13, 2018

Perhaps we should expose a method on the session interface for copying, so the dev can override deepcopy vs copy?

@dchevell

This comment has been minimized.

Contributor

dchevell commented Oct 13, 2018

@davidism do you mean along the lines of, @copy_current_request_context(deepcopy_session=True) ?

Do you think there are cases to want both behaviours? deepcopy seems safer, since the intent of the method isn't to provide references back to the original objects. I can imagine a case where a user would prefer deepcopy, but not one where they'd prefer a shallow copy.

@ThiefMaster

This comment has been minimized.

Member

ThiefMaster commented Oct 13, 2018

Maybe an attribute on the Flask class or the session interface (pointing to the copy function to use)? I'd rather subclass once instead of having to specify this again and again.

@dchevell

This comment has been minimized.

Contributor

dchevell commented Oct 13, 2018

I can do that. First, though, what's the benefit here? What does offering shallow vs deep copy achieve? Adding API surface increases cost (testability, complexity, documentation, etc.) and given the purpose of this helper function is for the fairly narrow use case of creating a context copy for use in threads or greenlets I'm not sure what shallow copies buy us. Happy to be overruled but I'd like to make sure before I touch any other code.

@dchevell

This comment has been minimized.

Contributor

dchevell commented Oct 16, 2018

Just checking in with @davidism to see if there's value / benefit in adding new API's to allow multiple copy behaviours - I'm still erring on the side of not, since this is already pretty edge-casey and it seems like an edge case of an edge case. Still, open to other opinions.

@okeuday

This comment has been minimized.

okeuday commented Oct 23, 2018

A separate problem is that flask.copy_current_request_context is calling RequestContext copy which does not copy the contents of environ. By not having a copy of environ when using copy_current_request_context with threads, it is possible for an old request to get data from a new request (in flask 1.0.2) (e.g., request.method). Such behavior seems to be justified by the code comment of the RequestContext copy function when it says "Because the actual request object is the same this cannot be used to move a request context to a different thread unless access to the request object is locked.". This indicates that flask.copy_current_request_context was previously meant to do a shallow copy and any deep copy functionality should require a boolean flag argument or a separate function.

I would like to use flask.copy_current_request_context if it was copying the request environ, so I would suggest adding a deep boolean keyword argument (defaulting to False) to the RequestContext copy function which is utilized when a boolean flag is passed to flask.copy_current_request_context. However, I know that is separate from @dchevell 's pursuit of a session copy.

@dchevell

This comment has been minimized.

Contributor

dchevell commented Oct 23, 2018

@okeuday To confirm, that's talking about deepcopying the request, not the session, right? (Of course, it would make sense to go all or nothing)

Can you deepcopy a request? I mean, you can't do it directly - you'll get TypeError: can't pickle _thread.lock objects. I'm sure this is solvable but unlike the session object it's not so trivial.

What are your thoughts on reverting this change back to the original shallow copy implementation, then opening a separate issue for a true deepcopy for both flask.request and flask.session as you and @davidism suggested? It seems like a worthy pursuit, but we're moving beyond the scope of this PR and the original issue.

@okeuday

This comment has been minimized.

okeuday commented Oct 23, 2018

@dchevell I would agree that a deep copy is best that handles the flask.request and flask.session. I am not sure if it needs a separate PR because they both seem related and it seems like people would likely want both to get a deep copy. flask.copy_current_request_context seems like a name that covers both a shallow copy and a deep copy, so it seems like it needs a function argument to switch between the different behavior. For the request copy, when not modifying RequestContext copy, I found it necessary to modify flask.copy_current_request_context so it looks like:

def _copy_current_request_context(function):
    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()
    # environ is used by headers too, so only a copy of environ is necessary
    reqctx.request.environ = reqctx.request.environ.copy()
    def wrapper(*args, **kwargs):
        with reqctx:
            return function(*args, **kwargs)
    return update_wrapper(wrapper, function)

It should be better to modify RequestContext copy to do the environ copy if a deep keyword argument is passed.

@dchevell

This comment has been minimized.

Contributor

dchevell commented Oct 23, 2018

@okeuday not sure I agree - the scope of this PR was simply, copies of the current request contain the previous session object. Shallow copy / deep copy covers the same area of code, but it's an entirely separate set of concerns. Other than touching the same code, I don't see a relationship between them.

I agree that a deepcopy has value, just not that it fits within the scope of #2935

@okeuday

This comment has been minimized.

okeuday commented Oct 23, 2018

@dchevell Ok, np, I moved the deep copy of the current request information to #2960

@dchevell

This comment has been minimized.

Contributor

dchevell commented Oct 23, 2018

If #2960 covers the need for offering an API for both shallow and deepcopying, I think we should revert the deepcopy change here back to the original shallow copy implementation, then a fix for #2960 should implement a deepcopy option which implements that behaviour for the request and session objects

@dchevell

This comment has been minimized.

Contributor

dchevell commented Nov 3, 2018

Any more thoughts on this? Given #2960 I would like to revert this to a shallow copy implementation, similar to the existing implementation for how the request object is copied, and then I'd be happy to follow up #2960 to investigate deep copy implementations for both.

/cc @davidism @ThiefMaster

@davidism

davidism requested changes Nov 3, 2018 edited

Show resolved Hide resolved flask/ctx.py Outdated
Show resolved Hide resolved flask/ctx.py Outdated
@davidism

This comment has been minimized.

Member

davidism commented Nov 3, 2018

Looking at the docs for RequestContext.copy():

Creates a copy of this request context with the same request object. This can be used to move a request context to a different greenlet. Because the actual request object is the same this cannot be used to move a request context to a different thread unless access to the request object is locked.

It's clear copying was never meant to use deepcopy, it requires the user to lock around multiple threads modifying. So let's go back to copy. In fact it may even be fine to just do new.session = session._get_current_object(), although I think copy is fine too, it prevents simple modifications at least.

@dchevell

This comment has been minimized.

Contributor

dchevell commented Nov 4, 2018

Thanks for the pointers @davidism - would you mind checking the latest diff to confirm if it matches what you were thinking?

Show resolved Hide resolved flask/ctx.py Outdated

dchevell added some commits Nov 4, 2018

@davidism davidism merged commit e08bcf9 into pallets:master Nov 4, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@davidism davidism added this to the 1.1 milestone Nov 4, 2018

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