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.

Sign up for GitHub

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

Jump to bottom

Document threading contract for Session class #2766

Closed
gward opened this issue Sep 10, 2015 · 13 comments
Closed

Document threading contract for Session class #2766

gward opened this issue Sep 10, 2015 · 13 comments
Labels
Documentation

Comments

@gward
Copy link

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
Copy link
Contributor

Why do you think the redirect cache is not threadsafe?

@Lukasa
Copy link
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
Copy link

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
Copy link
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. =)

@aaugustin aaugustin mentioned this issue Mar 4, 2016
Closed
@kennethreitz
Copy link
Contributor

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

@jseabold jseabold mentioned this issue Apr 19, 2016
Merged
@nickoala nickoala mentioned this issue May 1, 2016
Closed
This was referenced Jun 15, 2016
Closed
Closed
@marrony marrony mentioned this issue Oct 27, 2016
Merged
@jkhelil jkhelil mentioned this issue Mar 21, 2017
Open
@mmerickel
Copy link

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.

@the31k the31k mentioned this issue Jul 18, 2017
Merged
@reversefold
Copy link

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

@thelink2012 thelink2012 mentioned this issue Sep 8, 2017
Closed
guewen added a commit to guewen/connector that referenced this issue Dec 22, 2017
@guewen
Use a requests session to limit opened sessions
05c4d12 
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] psf/requests#2766
guewen added a commit to guewen/queue that referenced this issue Feb 8, 2018
@guewen
Use a requests session to limit opened sessions
597bfeb 
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] psf/requests#2766
guewen added a commit to guewen/queue that referenced this issue Feb 8, 2018
@guewen
Use a requests session to limit opened sessions
555e362 
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] psf/requests#2766
guewen added a commit to guewen/queue that referenced this issue May 25, 2018
@guewen
Use a requests session to limit opened sessions
370e01b 
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] psf/requests#2766
StefanRijnhart pushed a commit to EssentNovaTeam/connector that referenced this issue Jul 20, 2018
@guewen @StefanRijnhart
Use a requests session to limit opened sessions
6f5b507 
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] psf/requests#2766
rmoehn pushed a commit to elego/connector that referenced this issue Aug 20, 2018
@guewen @rmoehn
Use a requests session to limit opened sessions
7c530c3 
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] psf/requests#2766
rmoehn pushed a commit to elego/connector that referenced this issue Aug 21, 2018
@guewen @rmoehn
Use a requests session to limit opened sessions
bbb2576 
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] psf/requests#2766
StefanRijnhart pushed a commit to StefanRijnhart/connector that referenced this issue Aug 21, 2018
@guewen @StefanRijnhart
Use a requests session to limit opened sessions
19508e2 
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] psf/requests#2766
@madsmtm madsmtm mentioned this issue Feb 28, 2019
Open
@madsmtm
Copy link

madsmtm commented Feb 28, 2019

From what I can tell, @reversefold's PR culminated with urllib3/urllib3#1263, but looking at the work he did, it didn't sound impossible. So assuming we can fix that, what is the current state of thread safety in requests? I know that the redirect cache was removed in https://github.com/kennethreitz/requests/pull/4100, but @Lukasa mentioned that the whole Session class needs to be audited, is this something I could help with?

As a general question, is this downprioritized in favor of async stuff like https://github.com/kennethreitz/requests/issues/1390? (And if so, should I put my full attention there as well? 😉)

(For future reference, the note about thread safety was removed in 66293b8)

@madsmtm madsmtm mentioned this issue Feb 28, 2019
Closed
@christopherhesse christopherhesse mentioned this issue Jul 10, 2019
Closed
@keeth keeth mentioned this issue Feb 16, 2022
Open
@JulienPalard JulienPalard mentioned this issue Apr 10, 2022
Merged
@half-duplex half-duplex mentioned this issue May 14, 2022
Merged
4 tasks
@epwalsh epwalsh mentioned this issue Jun 27, 2022
Merged
5 tasks
@pradyunsg pradyunsg mentioned this issue Jul 15, 2022
Open
@septatrix septatrix mentioned this issue Nov 29, 2022
Open
Amund211 added a commit to Amund211/prism that referenced this issue Dec 22, 2022
@Amund211
Use a session object for requests
9b34681 
By using a session object we can reuse the tcp connection.
We use one session per domain.

There is some uncertainty about the thread safety of Session
(psf/requests#2766), but it seems to work well
when not making requests to many different domains (1 in our case).
@markkuleinio markkuleinio mentioned this issue Dec 26, 2022
Closed
@Dr-Emann
Copy link

Dr-Emann commented Feb 28, 2023 •
edited
Loading

Any official word on this? There's been a lot of foundational work in urllib3 to be thread safe, is the official recommendation still "one session per thread"?

@Wauplin Wauplin mentioned this issue Mar 1, 2023
Closed
@Wauplin Wauplin mentioned this issue Mar 16, 2023
Merged
4 tasks
@lyricnz lyricnz mentioned this issue May 24, 2023
Closed
@dimaqq
Copy link

dimaqq commented Jun 13, 2023

I too would like an official statement on this.

@david-gang
Copy link

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

looks like urllib is thread safe now: urllib3/urllib3#2661

@andamian andamian mentioned this issue Sep 22, 2023
Merged
@Dlougach Dlougach mentioned this issue Oct 4, 2023
Closed
bizzappdev pushed a commit to BizzAppDev-Systems/queue that referenced this issue Nov 23, 2023
@guewen @bizzappdev
Use a requests session to limit opened sessions
4d2e607 
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] psf/requests#2766
bizzappdev pushed a commit to BizzAppDev-Systems/queue that referenced this issue Nov 23, 2023
@guewen @bizzappdev
Use a requests session to limit opened sessions
3db43a2 
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] psf/requests#2766
nguyenminhchien pushed a commit to nguyenminhchien/queue that referenced this issue Nov 25, 2023
@guewen @nguyenminhchien
Use a requests session to limit opened sessions
2d08252 
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] psf/requests#2766
@sethmlarson
Copy link
Member

Closing this until a separate threading issue is shown in Requests, please open a new issue if that's the case.

@andrewsanchez andrewsanchez mentioned this issue Jun 10, 2024
Merged
@alicederyn alicederyn mentioned this issue Sep 2, 2024
Merged
4 tasks
alextremblay added a commit to uoft-networking/tools that referenced this issue Sep 18, 2024
@alextremblay
[core] add a threadpool method to APIBase
a113ca6 
this implementation was originally going to shard the API class instance into a separate deep-copied instance per thread, since conventional wisdom is that requests.Sessions (which APIBase subclasses) is not threadsafe.

However, digging into the details a bit (see psf/requests#2766 for details) it looks like requests.Session has been threadsafe since about august 2022
QuocDuong1306 pushed a commit to QuocDuong1306/queue that referenced this issue Sep 19, 2024
@guewen @QuocDuong1306
Use a requests session to limit opened sessions
4b6045e 
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] psf/requests#2766
alextremblay added a commit to uoft-networking/tools that referenced this issue Sep 21, 2024
@alextremblay
[core] add a threadpool method to APIBase
d70c71f 
this implementation was originally going to shard the API class instance into a separate deep-copied instance per thread, since conventional wisdom is that requests.Sessions (which APIBase subclasses) is not threadsafe.

However, digging into the details a bit (see psf/requests#2766 for details) it looks like requests.Session has been threadsafe since about august 2022
@alextremblay alextremblay mentioned this issue Sep 21, 2024
Merged
QuocDuong1306 pushed a commit to QuocDuong1306/queue that referenced this issue Sep 24, 2024
@guewen @QuocDuong1306
Use a requests session to limit opened sessions
78f157b 
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] psf/requests#2766
thienvh332 pushed a commit to thienvh332/queue that referenced this issue Oct 7, 2024
@guewen @thienvh332
Use a requests session to limit opened sessions
50c3cf0 
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] psf/requests#2766
@Dr-Emann
Copy link

Dr-Emann commented Nov 6, 2024

Trying one last time:

Is the official statement that using a single session object from multiple threads simultaneously is intended to be safe, we have a reasonable belief that it is, and that any cases where this is not true is considered to be a bug?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests