[3.6] bpo-31639: Use threads in http.server module. (GH-5018) #6203
Conversation
(cherry picked from commit 8bcfa02) Co-authored-by: Julien Palard <julien@palard.fr>
|
@JulienPalard: Backport status check is done, and it's a success ✅ . |
ned-deily
left a comment
There was a problem hiding this comment.
+0 I'm not sure that is appropriate for 3.6.x at this stage but I'm willing to be persuaded. BTW, I believe all of the comments apply to the 3.7 and master versions of the changes. I'm too lazy to duplicate.
| of which this module provides three different variants: | ||
| This class is identical to HTTPServer but uses threads to handle | ||
| requests by using the :class:`~socketserver.ThreadingMixin`. This | ||
| is usefull to handle web browsers pre-opening sockets, on which |
There was a problem hiding this comment.
As it's a backport, should I simply fix it in a new PR?
There was a problem hiding this comment.
Since the master and 3.7 versions are already merged, that would be simpler, sure.
| @@ -0,0 +1,2 @@ | |||
| http.server now exposes a ThreadedHTTPServer class and uses it when the | |||
| module is invoked to cope with web browsers pre-opening sockets. | |||
There was a problem hiding this comment.
"and uses it when the module is invoked" ? Doesn't the user have to explicitly use the new class?
There was a problem hiding this comment.
By "module is invoked" I mean running "python3 -m http.server". In the "python -m http.server" case, the ThreadedHTTPServer is used, to fix bpo-31639.
There was a problem hiding this comment.
So perhaps change "is invoked" to "is run with -m" ?
|
|
||
| The :class:`HTTPServer` and :class:`ThreadedHTTPServer` must be given | ||
| a *RequestHandlerClass* on instantiation, of which this module | ||
| provides three different variants: |
There was a problem hiding this comment.
Needs a version added comment.
There was a problem hiding this comment.
You're right. Hum, what is the protocol to decide which version to put it as it's being backported? I initially though 3.8 but as it's backported ... maybe 3.6 but it's wrong (as not precise enough, it was not present in 3.6.*), so maybe versionadded:: 3.6.6?
There was a problem hiding this comment.
I think 3.6.6 is the right answer in this case for all branches.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
@ned-deily thanks for the feedback. I think it's ok for 3.6 (not as a language bug fix I agree, but as a usability fix): I encontered the issue multiple time by using "python3 -m http.server", typically to test the doc. I expect other people to find the bug too. |
|
OK, I guess this is OK for 3.6. If you want to address some or all of the comments in a follow-on PR for master and 3.7, that's fine. |
|
As discussed with @vstinner, I'll finally won't backport it to 3.6: introducing a new class (changing the API) is something we should not do. It's still fixable without changing the API by hiding the |
|
@JulienPalard @miss-islington Thanks for this PR. However what do you think about changing the class name
|
(cherry picked from commit 8bcfa02)
Co-authored-by: Julien Palard julien@palard.fr
https://bugs.python.org/issue31639