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.

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

Merged
merged 4 commits into from
May 1, 2019

Conversation

PetterS
Copy link
Contributor

@PetterS PetterS commented Oct 5, 2018

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

@PetterS
Copy link
Contributor Author

PetterS commented Oct 6, 2018

I have signed the CLA before, but the bot has not updated this PR either way.
Also, the Azure failing CI seems to be incorrect (other PRs were also failing when I submitted this.)

@PetterS
Copy link
Contributor Author

PetterS commented Oct 11, 2018

@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?

@deeredman1991
Copy link

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;

    def write(self, b):
        try: #Not Standard Python
            self._sock.sendall(b)
        except ConnectionAbortedError: #Not Standard Python
            self._sock.close() #Not Standard Python
        with memoryview(b) as view:
            return view.nbytes

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?

@berkerpeksag berkerpeksag self-requested a review October 12, 2018 18:53
@zooba
Copy link
Member

zooba commented Oct 13, 2018

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.)

@zooba
Copy link
Member

zooba commented Oct 13, 2018

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.

Copy link
Member

@berkerpeksag berkerpeksag left a 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.

@PetterS
Copy link
Contributor Author

PetterS commented Oct 13, 2018

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.

@deeredman1991
Copy link

deeredman1991 commented Oct 13, 2018

@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.

@PetterS
Copy link
Contributor Author

PetterS commented Oct 13, 2018

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.

@PetterS
Copy link
Contributor Author

PetterS commented Oct 19, 2018

@vadmium Did you have a chance to look at this? Thanks in advance!

@vadmium
Copy link
Member

vadmium commented Oct 19, 2018

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.

@vadmium
Copy link
Member

vadmium commented Nov 4, 2018

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?

@PetterS
Copy link
Contributor Author

PetterS commented Nov 4, 2018

Personally, I don't feel that this is worth a warning, either. I can revert back to 9dc6eb5 if that can be the consensus.

@PetterS PetterS changed the title bpo-34547: Handle client connection terminations in wsgiref. bpo-27682: Handle client connection terminations in wsgiref. Nov 4, 2018
@PetterS
Copy link
Contributor Author

PetterS commented Nov 4, 2018

The bug I created was closed as a duplicate, so I repointed this PR to the other issue.

birthdaysgift added a commit to birthdaysgift/mimic that referenced this pull request Nov 19, 2018
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
@PetterS
Copy link
Contributor Author

PetterS commented Nov 20, 2018

@vadmium What do you think is the way forward here? Reverting to 9dc6eb5?

@vadmium
Copy link
Member

vadmium commented Nov 26, 2018

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.

@aeltawela
Copy link

this issue is running since 2016, any plan to fix it in the next releases?

@PetterS
Copy link
Contributor Author

PetterS commented Dec 16, 2018

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.
@PetterS
Copy link
Contributor Author

PetterS commented Jan 6, 2019

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.

@aeltawela
Copy link

It is cool that the changes are ready, but may I know in which release will be these changes included?

@PetterS
Copy link
Contributor Author

PetterS commented Feb 20, 2019

@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?

@SakisHous
Copy link

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?

@PetterS
Copy link
Contributor Author

PetterS commented May 15, 2019

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.

@tirkarthi
Copy link
Member

It has been backported and will be part of 3.7.4.

@berkerpeksag
Copy link
Member

3.7.3 was released on March 25. This fix will be released with 3.7.4.

@changeling
Copy link

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:

PEP 537 -- Python 3.7 Release Schedule

@afvca
Copy link

afvca commented Jul 9, 2019

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.

@wkoot
Copy link

wkoot commented Jul 10, 2019

Works well for me on 3.7.4 with Windows 10, running from subshell in PyCharm/IntelliJ IDEA.

@afvca
Copy link

afvca commented Jul 10, 2019

Well, turns out I was testing with Mozilla Firefox and was getting these errors.

Socket error: [WinError 10053] An established connection was aborted by the software in your host machine

Tested with Google Chrome and it is working fine. Might be a Firefox bug..

@KANE-99
Copy link

KANE-99 commented Jul 21, 2019

Well, turns out I was testing with Mozilla Firefox and was getting these errors.

Socket error: [WinError 10053] An established connection was aborted by the software in your host machine

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.

@PetterS
Copy link
Contributor Author

PetterS commented Jul 22, 2019

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.

@boshng95
Copy link

boshng95 commented Aug 4, 2019

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

@thuanvn
Copy link

thuanvn commented Aug 19, 2019

I had same situation with boshng95. just ignore & go ahead for now

@JoeKhoa
Copy link

JoeKhoa commented Jan 8, 2020

same error in Python.3.8 and Django2.2.5
Exception happened during processing of request from ('127.0.0.1', 53745)
Traceback (most recent call last):
File "C:\Users\Lilti\Miniconda3\envs\mydjangoenv\lib\socketserver.py", line 650, in process_request_thread
self.finish_request(request, client_address)
File "C:\Users\Lilti\Miniconda3\envs\mydjangoenv\lib\socketserver.py", line 360, in finish_request
self.RequestHandlerClass(request, client_address, self)
File "C:\Users\Lilti\Miniconda3\envs\mydjangoenv\lib\socketserver.py", line 720, in init
self.handle()
File "C:\Users\Lilti\Miniconda3\envs\mydjangoenv\lib\site-packages\django\core\servers\basehttp.py", line 171, in handle
self.handle_one_request()
File "C:\Users\Lilti\Miniconda3\envs\mydjangoenv\lib\site-packages\django\core\servers\basehttp.py", line 179, in handle_one_request
self.raw_requestline = self.rfile.readline(65537)
File "C:\Users\Lilti\Miniconda3\envs\mydjangoenv\lib\socket.py", line 669, in readinto
return self._sock.recv_into(b)
ConnectionAbortedError: [WinError 10053] An established connection was aborted by the software in your host machine

@hackzaid
Copy link

Any update on this issue, running python 3.8.4 but same error;
ConnectionAbortedError: [WinError 10053] An established connection was aborted by the software in your host machine

Tested my Django app on chrome works fine, firefox not working

@khorammfar
Copy link

i have same error in python 3.8.6rc1 Django 3.1 !!! window and linux
how you guys fix this ?

@PetterS
Copy link
Contributor Author

PetterS commented Nov 25, 2020

Alright, I don't use Windows alot any more, but perhaps I can find the time to look into this again.

@stonetalons
Copy link

This issue do annoy me A LOT! I have tried the following solution in socketserver.py :
def write(self, b):
try: #Not Standard Python
self._sock.sendall(b)
except ConnectionAbortedError: #Not Standard Python
self._sock.close() #Not Standard Python
with memoryview(b) as view:
return view.nbytes
while it doesn't work.(My environment: Python:3.6.4,django 3.1.2)
So...I rewirite the readinto function in socket.py

        except error as e:
            if e.args[0] in _blocking_errnos:
                return None
            if e.args[0]==10053:
                return None
            raise

OK my terminal won‘t report 10053 error anymore.#EMBARRASSED FACE
(STILL LOOKING FOR A REAL SOLUTION!)

@PetterS
Copy link
Contributor Author

PetterS commented Dec 3, 2020

This PR changed wsgiref/handlers.py but the new stack trace above does not mention this file. Has something in Django changed so that it does not use wsgiref any more? Compare to the stack trace in https://bugs.python.org/issue27682

@PetterS
Copy link
Contributor Author

PetterS commented Dec 3, 2020

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

@PetterS
Copy link
Contributor Author

PetterS commented Dec 3, 2020

FYI I created django/django#13743

felixxm pushed a commit to PetterS/django that referenced this pull request Dec 14, 2020
@PetterS
Copy link
Contributor Author

PetterS commented Dec 14, 2020

For people seeing this in Django: it should be better in the next version.

@krzysiaczeks
Copy link

krzysiaczeks commented Feb 19, 2021

I,ve got same error in Windows 8.1, Python 3.9.1, Django 3.1.7. No matter Firefox or Chrome :/

    [19/Feb/2021 11:11:54] "GET / HTTP/1.1" 200 2222
    Not Found: /favicon.ico
    ----------------------------------------
    Exception occurred during processing of request from ('127.0.0.1', 50643)
    Traceback (most recent call last):
      File "C:\Users\Krzyś\AppData\Local\Programs\Python\Python39\lib\socketserver.py", line 650, in process_request_thread
        self.finish_request(request, client_address)
      File "C:\Users\Krzyś\AppData\Local\Programs\Python\Python39\lib\socketserver.py", line 360, in finish_request
        self.RequestHandlerClass(request, client_address, self)
      File "C:\Users\Krzyś\AppData\Local\Programs\Python\Python39\lib\socketserver.py", line 720, in __init__
        self.handle()
      File "C:\Users\Krzyś\Documents\Django [szkolenia]\djangostocks\venv\lib\site-packages\django\core\servers\basehttp.py", line 174, in handle
        self.handle_one_request()
      File "C:\Users\Krzyś\Documents\Django [szkolenia]\djangostocks\venv\lib\site-packages\django\core\servers\basehttp.py", line 182, in handle_one_request
        self.raw_requestline = self.rfile.readline(65537)
      File "C:\Users\Krzyś\AppData\Local\Programs\Python\Python39\lib\socket.py", line 704, in readinto
        return self._sock.recv_into(b)
    ConnectionAbortedError: [WinError 10053] Nawiązane połączenie zostało przerwane przez oprogramowanie zainstalowane w komputerze-hoście
    ----------------------------------------

@Charlink3
Copy link

It has been backported and will be part of 3.7.4.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.