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

Document threading contract for Session class #2766

Open
gward opened this Issue Sep 10, 2015 · 7 comments

Comments

Projects
None yet
7 participants
@gward

gward commented Sep 10, 2015

Right now, it's quite difficult to figure out if the Session class is threadsafe or not. The docs don't say, apart from a "thread-safe" bullet on the home page. Google led me to http://stackoverflow.com/questions/18188044/is-the-session-object-from-pythons-requests-library-thread-safe, whose first answer boils down to "be very careful".

Inspired by that SO author, I've been auditing the code myself, and have come to the conclusion that Session is probably not threadsafe. (The use of self.redirect_cache set off red flags for me.) Reading through other requests bug reports, I see maintainers recommending one Session per thread, which implies that it's not threadsafe.

The documentation should clarify this and recommend how to use Session in multithreaded programs. Possible text:

Session is not threadsafe. If you are using requests with an explicit Session
object in a multithreaded program, you should create one Session per thread.

If that's accurate, let me know and I'll submit a PR.

Also, I think the "thread-safe" bullet should be removed from the home page, or maybe replaced by "thread-safe in certain circumstances".

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Sep 10, 2015

Why do you think the redirect cache is not threadsafe?

@Lukasa

This comment has been minimized.

Member

Lukasa commented Nov 5, 2015

I'd go further: I think the redirect cache is a bad idea. I still think that.

I think re-adding thread-safety would be a good idea. We can get away with it, I think, by locking around the CookieJar (which should be the only other thing that is a risk here). That's a cheap-and-easy way to get something approximating thread safety, even if it doesn't necessarily get great performance.

@aaugustin

This comment has been minimized.

aaugustin commented Mar 3, 2016

It would be pretty cool if requests.Session was guarateed to be safe.

This would make that sort of code safe by default.

This sounds better than pestering every maintainer of every API client based on requests to make their Session objects thread-local :-)

I don't expect a large overhead from locking around writes to the cookie jar. Requests is supposed to spend most of its time waiting for I/O.

@Lukasa

This comment has been minimized.

Member

Lukasa commented Mar 3, 2016

The cookie jar, sadly, is not the only concern: the entire Session class needs auditing for thread safety. It's not a bad idea, it's just a matter of finding someone with the time to spare to do it. =)

@kennethreitz

This comment has been minimized.

Member

kennethreitz commented Mar 4, 2016

Sounds like someone should write a tool that makes this really easy :P

@mmerickel

This comment has been minimized.

mmerickel commented May 17, 2017

What is the motivation for the "thread-safety" bullet point on the main page? I can't find the word "thread" almost anywhere in the documentation and definitely nothing about thread safety. This issue alongside several others indicate that the session is not actually thread-safe. The bullet point really should be removed as it seems unfounded right now.

@reversefold

This comment has been minimized.

reversefold commented Aug 28, 2017

I've opened an issue with urllib3 as the main underlying issue appears to be with PoolManager and HTTPConnectionPool, both of which are part of urllib3 and not requests.
urllib3/urllib3#1252

guewen added a commit to guewen/connector that referenced this issue Dec 22, 2017

Use a requests session to limit opened sessions
We have some issues with a very high throughput of jobs and Odoo's
anonymous sessions. Every job pushed to Odoo uses a new anonymous
sessions, which creates a tremendous amount of session files.

Using a requests.session means that the session will keep the same
cookie once it receives the first response.

There is 2 problems with this implementation though:

* requests.Session is not proved to be threadsafe [0], but maybe we can
manually retrieve the session id and craft the next messages with it.
Anyway as they are all anonymous, if we lose a session at some point
there is no big deal.
* the main issue is that we don't wait for an answer so until we have a
requests responding before the timeout we won't have any session id

[0] requests/requests#2766

guewen added a commit to guewen/queue that referenced this issue Feb 8, 2018

Use a requests session to limit opened sessions
We have some issues with a very high throughput of jobs and Odoo's
anonymous sessions. Every job pushed to Odoo uses a new anonymous
sessions, which creates a tremendous amount of session files.

Using a requests.session means that the session will keep the same
cookie once it receives the first response.

There is 2 problems with this implementation though:

* requests.Session is not proved to be threadsafe [0], but maybe we can
manually retrieve the session id and craft the next messages with it.
Anyway as they are all anonymous, if we lose a session at some point
there is no big deal.
* the main issue is that we don't wait for an answer so until we have a
requests responding before the timeout we won't have any session id

[0] requests/requests#2766

guewen added a commit to guewen/queue that referenced this issue Feb 8, 2018

Use a requests session to limit opened sessions
We have some issues with a very high throughput of jobs and Odoo's
anonymous sessions. Every job pushed to Odoo uses a new anonymous
sessions, which creates a tremendous amount of session files.

Using a requests.session means that the session will keep the same
cookie once it receives the first response.

There is 2 problems with this implementation though:

* requests.Session is not proved to be threadsafe [0], but maybe we can
manually retrieve the session id and craft the next messages with it.
Anyway as they are all anonymous, if we lose a session at some point
there is no big deal.
* the main issue is that we don't wait for an answer so until we have a
requests responding before the timeout we won't have any session id

[0] requests/requests#2766

guewen added a commit to guewen/queue that referenced this issue May 25, 2018

Use a requests session to limit opened sessions
We have some issues with a very high throughput of jobs and Odoo's
anonymous sessions. Every job pushed to Odoo uses a new anonymous
sessions, which creates a tremendous amount of session files.

Using a requests.session means that the session will keep the same
cookie once it receives the first response.

There is 2 problems with this implementation though:

* requests.Session is not proved to be threadsafe [0], but maybe we can
manually retrieve the session id and craft the next messages with it.
Anyway as they are all anonymous, if we lose a session at some point
there is no big deal.
* the main issue is that we don't wait for an answer so until we have a
requests responding before the timeout we won't have any session id

[0] requests/requests#2766

StefanRijnhart added a commit to EssentNovaTeam/connector that referenced this issue Jul 20, 2018

Use a requests session to limit opened sessions
We have some issues with a very high throughput of jobs and Odoo's
anonymous sessions. Every job pushed to Odoo uses a new anonymous
sessions, which creates a tremendous amount of session files.

Using a requests.session means that the session will keep the same
cookie once it receives the first response.

There is 2 problems with this implementation though:

* requests.Session is not proved to be threadsafe [0], but maybe we can
manually retrieve the session id and craft the next messages with it.
Anyway as they are all anonymous, if we lose a session at some point
there is no big deal.
* the main issue is that we don't wait for an answer so until we have a
requests responding before the timeout we won't have any session id

[0] requests/requests#2766

StefanRijnhart added a commit to StefanRijnhart/connector that referenced this issue Aug 17, 2018

Use a requests session to limit opened sessions
We have some issues with a very high throughput of jobs and Odoo's
anonymous sessions. Every job pushed to Odoo uses a new anonymous
sessions, which creates a tremendous amount of session files.

Using a requests.session means that the session will keep the same
cookie once it receives the first response.

There is 2 problems with this implementation though:

* requests.Session is not proved to be threadsafe [0], but maybe we can
manually retrieve the session id and craft the next messages with it.
Anyway as they are all anonymous, if we lose a session at some point
there is no big deal.
* the main issue is that we don't wait for an answer so until we have a
requests responding before the timeout we won't have any session id

[0] requests/requests#2766

rmoehn added a commit to elego/connector that referenced this issue Aug 20, 2018

Use a requests session to limit opened sessions
We have some issues with a very high throughput of jobs and Odoo's
anonymous sessions. Every job pushed to Odoo uses a new anonymous
sessions, which creates a tremendous amount of session files.

Using a requests.session means that the session will keep the same
cookie once it receives the first response.

There is 2 problems with this implementation though:

* requests.Session is not proved to be threadsafe [0], but maybe we can
manually retrieve the session id and craft the next messages with it.
Anyway as they are all anonymous, if we lose a session at some point
there is no big deal.
* the main issue is that we don't wait for an answer so until we have a
requests responding before the timeout we won't have any session id

[0] requests/requests#2766

rmoehn added a commit to elego/connector that referenced this issue Aug 21, 2018

Use a requests session to limit opened sessions
We have some issues with a very high throughput of jobs and Odoo's
anonymous sessions. Every job pushed to Odoo uses a new anonymous
sessions, which creates a tremendous amount of session files.

Using a requests.session means that the session will keep the same
cookie once it receives the first response.

There is 2 problems with this implementation though:

* requests.Session is not proved to be threadsafe [0], but maybe we can
manually retrieve the session id and craft the next messages with it.
Anyway as they are all anonymous, if we lose a session at some point
there is no big deal.
* the main issue is that we don't wait for an answer so until we have a
requests responding before the timeout we won't have any session id

[0] requests/requests#2766

StefanRijnhart added a commit to StefanRijnhart/connector that referenced this issue Aug 21, 2018

Use a requests session to limit opened sessions
We have some issues with a very high throughput of jobs and Odoo's
anonymous sessions. Every job pushed to Odoo uses a new anonymous
sessions, which creates a tremendous amount of session files.

Using a requests.session means that the session will keep the same
cookie once it receives the first response.

There is 2 problems with this implementation though:

* requests.Session is not proved to be threadsafe [0], but maybe we can
manually retrieve the session id and craft the next messages with it.
Anyway as they are all anonymous, if we lose a session at some point
there is no big deal.
* the main issue is that we don't wait for an answer so until we have a
requests responding before the timeout we won't have any session id

[0] requests/requests#2766
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment