-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
bpo-27682: Handle client connection terminations in wsgiref. #9713
Conversation
I have signed the CLA before, but the bot has not updated this PR either way. |
@berkerpeksag You seem to have touched wsgiref last. I know it hasn't been developed a lot the last years, but it is still used quite a lot. Do you think you could have a look at this? |
I was able to fix the issue on my end by modifying Python36/Lib/socketserver.py I modified the write method (at around line 800) of the _SocketWriter() class to the following;
I am not sure if this is going to cause other issues or not but; it stopped the error messages/stacktrace I wanted to let you guys know about it in case it could help... I am not too familiar with the python source code... If you guys foresee any issues I might have with this modified code off the top of your head; could you give me a heads up? |
I'm keen to see this fixed - it's been an annoyance for quite a while :) Should we log a message on the connection abort? This is meant to be a reference server, so logging something shouldn't hurt, and I'd hate to be the person who has to diagnose why their aborted connections are being silently handled (or something one step removed that would be obvious if they knew the connections were aborting). I also wonder if there's a more narrow place we can handle the exception? (It's been a while since I looked closely at the stack traces and don't have any handy now, but if there's one place where it always occurs rather than "anywhere in the WSGI app", it'd be better to handle it there.) |
Also, you can ignore the Azure Pipelines failure here. It should work on re-run (and possibly with a merge from master) - there was a problem with the configuration that has since been fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that catching this exception in a more specific place would be better. I think SimpleHandler._write()
might be a good place to do this. Also, we can reuse the existing warn()
to log something to users:
def _write(self,data):
from warnings import warn
try:
result = self.stdout.write(data)
except ConnectionAbortedError:
warn(...)
return
else:
if result is None or result == len(data):
return
...
I'm not sure about catching ConnectionAbortedError
in socketserver. I think it's the client's responsibility to catch such an exception, but it might be a good idea to check how other server implementations handle this case.
Thanks for the feedback! I am hestitating a little about warning for a ConnectionAbortedError since that is a pretty normal thing for a web server, but its is a large improvement over 100+ lines of stack traces, so I guess we could go with that. PR updated. It works nicely with Django’s test server, which is my main concern. |
@PetterS What about a flag somewhere like "os.environ['DISPLAY_CONNECTION_ABORTED_WARING'] = True"? Idk if os.environ would be the right place to put it or not but it's an idea... or instead of making it a "warning" just treat it as a notification. |
I don't think we want to introduce a environment variable for this. I am happy with the current change – it writes two lines to the Django test server log when the browser closes the connection. Further, Python already has a system for disabling specific warnings. |
@vadmium Did you have a chance to look at this? Thanks in advance! |
Hi, I haven't had a chance to have a good look at the code yet. Maybe the weekend after this one. I agree that exceptions like connection abort, that are caused by the remote client or network, should be handled more gracefully and quietly. |
Using the warnings module rather than the WSGI stderr logging functionality seems a bit surprising. I agree that it is nice that you can enable and disable warnings and convert them to exceptions. If you turn on multiple warnings (python -Walways), I expect you might see the same warning repeated as each write call is attempted. If the application is sending a large response, or doing significant computation, it might be nice to abort sooner rather than letting it go through the response. I think the earlier version that caught the exception higher up (revision 9dc6eb5) is a better behaviour. But in these cases I normally silence all exceptions derived from ConnectionError. On Linux clients and servers, I have never seen ECONNABORTED, but tend to see EPIPE and ECONNRESET instead. Assuming the more general problem with the double exceptions gets fixed (see https://bugs.python.org/issue29183), then the exception log should revert back to something like the first 16 lines from Petter’s post in https://bugs.python.org/issue34547. If people prefer something is logged, but think the 16-line backtrace is still too much, perhaps just log the exception message in a single line to the WSGI’s stderr? |
Personally, I don't feel that this is worth a warning, either. I can revert back to 9dc6eb5 if that can be the consensus. |
The bug I created was closed as a duplicate, so I repointed this PR to the other issue. |
The problem that I intented to describe in this TODO is a bug of python standart library (wsgi). More info about it can be found here: https://bugs.python.org/issue27682 https://bugs.python.org/issue29183 python/cpython#9713
@vadmium What do you think is the way forward here? Reverting to 9dc6eb5? |
I think it is more important to fix the double exceptions first, like I suggested at https://bugs.python.org/issue29183#msg329223. That would bring the behaviour back to how it used to be when wsgiref was added to Python. If people still want to change how ConnectionAbortedError (ECONNABORTED) is logged, going back to revision 9dc6eb5 is a good starting point. But I would encourage you to include similar exceptions such as EPIPE and ECONNRESET. And am in two (or three) minds about whether it is better to hide these conditions completely, log a backtrace showing where the server discovered the broken connection, or perhaps log a short message on a single line. Sometimes the verbose logs get in the way of more interesting log messages; other times they are useful for diagnosing your code. This tradeoff is similar to other servers based on socketserver (for example “python -m http.server”) that log the full exception backtrace, and more generally the default handling of BrokenPipeError and KeyboardInterrupt in the Python interpreter. |
this issue is running since 2016, any plan to fix it in the next releases? |
Yes, I still have a plan to try and get this submitted. I got feedback for my initial version, changed it, but the latest feedback from vadmium suggests that I should go back to my original version 9dc6eb5 and take it from there. |
Previously, long stack traces with Python TypeErrors and AttributeErrors were printed (see bug). The wsgiref server is used quite a lot in the community, notably by the Django development server. I see these huge stack traces daily.
I have now reverted to the original state of this PR. It does not address everything (double exceptions in general, EPIPE, ECONNRESET ,…). Still, I think this would be good to merge. It is an annoyance for Django developers on Windows. |
It is cool that the changes are ready, but may I know in which release will be these changes included? |
@ahmedabt A maintainer will have to look at this and merge it first. Like us, they are also volunteers, so there is no timeline. @vadmium Since this has been going on for six months and is pretty useful as is, any change that we can merge this in order for it to make 3.8 and postpone additional changes until later? |
I encounter the same problem with Python 3.7.3 and Django 2.2.1 in Windows 10. Should I wait for Python 3.8? |
This will be backported to the most recent version of 3.7. |
It has been backported and will be part of 3.7.4. |
3.7.3 was released on March 25. This fix will be released with 3.7.4. |
Really looking forward to June 24. @berkerpeksag and @PetterS, thank y'all again for cleaning this up. It's been a long running thorn in folks' sides. For anyone not familiar with the release schedule, here's the relevant link including 3.7.4 projected release date: |
I believe this is still not fixed...Upgraded to Python 3.7.4 but still getting these warnings when running the development server in Windows 10. |
Works well for me on 3.7.4 with Windows 10, running from subshell in PyCharm/IntelliJ IDEA. |
Well, turns out I was testing with Mozilla Firefox and was getting these errors.
Tested with Google Chrome and it is working fine. Might be a Firefox bug.. |
I Encountered exactly opposite, Works fine without any issue with Firefox while got this ConnectionAbortedError error on Chrome. |
This is the discussion of a merged pull request. Could we raise these additional issues in the bug please? I will comment there. The remaining log message probably make sense to fix in Django. |
The upgrade from Python 3.7.3 to 3.7.4 remove the error message of AttributeError & TypeError. However, there is still one error message unable to resolve. For my case, I had upgraded my Python to 3.7.4 & Django 2.2.4 with Windows 10 & run the server using Firefox Developer Edition. Still having an error as shown below: ConnectionAbortedError: [WinError 10053] An established connection was aborted by the software in your host machine |
I had same situation with boshng95. just ignore & go ahead for now |
same error in Python.3.8 and Django2.2.5 |
Any update on this issue, running python 3.8.4 but same error; Tested my Django app on chrome works fine, firefox not working |
i have same error in python 3.8.6rc1 Django 3.1 !!! window and linux |
Alright, I don't use Windows alot any more, but perhaps I can find the time to look into this again. |
This issue do annoy me A LOT! I have tried the following solution in socketserver.py :
OK my terminal won‘t report 10053 error anymore.#EMBARRASSED FACE |
This PR changed |
But I think this error can be fixed by adding ConnectionAbortedError here in Django: https://github.com/django/django/blob/c6581a40be3bb4c1e13861f0adbb3fe01f09107f/django/core/servers/basehttp.py#L55 |
FYI I created django/django#13743 |
…tionResetError errors. See https://bugs.python.org/issue27682 and python/cpython#9713
For people seeing this in Django: it should be better in the next version. |
I,ve got same error in Windows 8.1, Python 3.9.1, Django 3.1.7. No matter Firefox or Chrome :/
|
Hello, I have the same problem And I have the same version 3.7.4, this issue happened a week ago, Do you solve the problem, can you help me? |
Previously, long stack traces with Python TypeErrors and AttributeErrors
were printed (see bug).
The wsgiref server is used quite a lot in the community, notably by the
Django development server. I see these huge stack traces daily.
https://bugs.python.org/issue27682