Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upDocument threading contract for Session class #2766
Comments
This comment has been minimized.
This comment has been minimized.
|
Why do you think the redirect cache is not threadsafe? |
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
It would be pretty cool if 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. |
This comment has been minimized.
This comment has been minimized.
|
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. =) |
This comment has been minimized.
This comment has been minimized.
|
Sounds like someone should write a tool that makes this really easy :P |
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
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. |
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
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
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
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
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
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
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
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
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
This comment has been minimized.
This comment has been minimized.
|
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 As a general question, is this downprioritized in favor of (For future reference, the note about thread safety was removed in 66293b8) |
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_cacheset 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:
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".