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

Allow for custom requests sessions #22

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

michaelweiser
Copy link

The requests module internally works with Session objects. These can be
customised in behaviour e.g. using retry adapters. This is extremely
useful for error handling because the user application doesn't even get
to see and need to handle errors until a configurable number of retries
with optional backoff has been tried. Unfortunately, the module-level
API of the requests module does not allow to supply such a customised
Session object. Instead it creates a new Session for each request and
needs to use it as a context handler to make sure it leaks no sockets -
see
https://github.com/psf/requests/blob/143150233162d609330941ec2aacde5ed4caa510/requests/api.py#L57
for details. The module-level API is a very thin shim which direcly
hands all requests to this per-request Session object.

This change switches the Cortex API layer to use a Session object for
requests directly and adds a parameter so users can provide their own
customised Session object.

As a side-effect, this switches the module to using a persistent Session
object that is re-used across requests. This should be more efficient in
that it can re-use an established connection and potentially connection
pooling.

Addendum:

  • see scVENUS/PeekabooAV@8964236 for this functionality in action
  • it does not appear to me that cortex4py makes any assumptions about requests being isolated and their sessions temporary. Any feedback on whether this change would break any assumptions about module idempotence would be appreciated.

@Jack28
Copy link

Jack28 commented Oct 26, 2020

I have tried this out and discovered that when submitting requests with a session cookie, the API also requires an XRSF token. The WebUI learns this token from the /api/user/current endpoint that can be accessed with just a session cookie but without an XSRF token. cortex4py without this PR does not run into this problem because it starts a new session on
every request by submitting the bearer token and never learning any session cookie from the response. Our requests session object however will learn the session cookie automatically from any request and there's no official way to reset the session cookie store.

We have tried to learn the cookie the same way the web ui does which seems to work but requests are still rejected by the API. Reason unknown.

But it turns out that the requests module has what seems to be a bug that setting the session cookie to any value will effectively mask the one returned by the server in following requests. Use that workaround to the workaround for now.

I've implemented this workaround for now: michaelweiser/PeekabooAV@f5f5e9d

What is the recommended way to proceed here?
How should the API treat the XSRF token and SESSION cookie?

@michaelweiser
Copy link
Author

I have changed the patch so it leaves the default behaviour of cortex4py alone. See scVENUS/PeekabooAV@fafa69b how the user can provide a forgetful cookie policy to avoid any possibility of confusing Cortex.

I would mention that while I remember to have reproduced the issue of Cortex setting a SESSION_COOKIE before handing us a CORTEX-XSRF-TOKEN and then refusing to talk to use as described above, I can no longer, neither with the training vm 4.0 nor the docker container image. Good news, I'd say.

@michaelweiser
Copy link
Author

Ah, I see another patch went in here as well, dealing with an annoying AttributeError exception when trying to run a nonexistant analyzer. Let me know whether this should get its own PR.

@michaelweiser
Copy link
Author

Any news on this? I still have a need for this functionality.

The requests module internally works with Session objects. These can be
customised in behaviour e.g. using retry adapters. This is extremely
useful for error handling because the user application doesn't even get
to see and need to handle errors until a configurable number of retries
with optional backoff has been done.

Unfortunately, the module-level API of the requests module does not
allow to supply such a customised Session object. Instead it creates a
new Session for each request and needs to use it as a context handler to
make sure it leaks no sockets - see
https://github.com/psf/requests/blob/143150233162d609330941ec2aacde5ed4caa510/requests/api.py#L57
for details. The module-level API is a very thin shim which directly
hands all requests to this per-request Session object.

This change switches the Cortex API layer to allow usage of a Session
object for requests and adds a parameter so users can provide their own
customised Session object.

To keep default behaviour unchanged, a fresh internal session object is
created for each request. This is done so the session does indeed not
accumulate state over time. For example this prevents learning of
cookies from responses which has been observed to confuse Cortex on
subsequent requests.

In the case of a user-supplied session it is the responsibility of the
user to configure it in such a way that it serves their purpose and does
not show behaviour that confuses Cortex.

Signed-off-by: Michael Weiser <[email protected]>
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