socket hang up and keep-alive connections

[x-yuri]

Current behavior:

Requests that result in a redirect fail with "socket hang up" error.

Desired behavior:

They succeed.

Steps to reproduce: (app code and test code)

$ python -m venv env
$ . env/bin/activate
$ pip install Django==2.0.2
$ django-admin startproject p1
$ cd p1
$ ./manage.py runserver

cypress/integration/1.spec.js:

describe('redirect requests', () => {
    it('succeeds', () => {
        cy.visit('http://localhost:8000/admin/');
    });
});

Versions

Cypress 3.4.0, Django 2.0.2

More info

This is supposedly caused by a bug in Django development web server, which was fixed in 2.1.4. Before this fix Django unconditionally closed the connection after the first request.

The thing is at 3.3.0 Cypress switched to keep-alive connections, so now it tries to do a second request (to the URL it was given to in the first one) over the same connection. But at about the same time Django closes the connection, which results in the error in the title.

I'm going to confirm in the near future if switching to the latest Django handles the issue.

Then, there's an open issue in Python's requests library (which is not used here, but still), that claims that a client that doesn't establish a new connection in this case is not well-behaved. And there are open issues in request package which Cypress uses. So the bug seems to be on the client side as well. Or rather on the client side. I'm not sure.

[flotwig]

Thank you for the detailed bug report!

In HTTP/1.1, Keep-Alive support is implicit unless Connection: close is sent. I set up Django by following your instructions and got the following headers from /admin:

As you can see, there is no Connection: close header, so Cypress attempts to use the keep-alive session that should implicitly exist. The Django maintainers mention that this is the bug:

Ticket #25619 changed the default protocol to HTTP/1.1 but did not
properly implement keep-alive. As a "fix" keep-alive was disabled in
ticket #28440 to prevent clients from hanging (they expect the server to
send more data if the connection is not closed and there is no content
length set).

The combination of those two fixes resulted in yet another problem:
HTTP/1.1 by default allows a client to assume that keep-alive is
supported unless the server disables it via 'Connection: close' -- see
RFC2616 8.1.2.1 for details on persistent connection negotiation. Now if
the client receives a response from Django without 'Connection: close'
and immediately sends a new request (on the same tcp connection) before
our server closes the tcp connection, it will error out at some point
because the connection does get closed a few milli seconds later.

I don't think that this is a bug in Cypress - Cypress receives the headers instructing it to redirect and immediately responds on the same socket, which is correct behavior. The bug is in Django for not actually keeping the socket alive.

---

Regardless, retrying one time on an ECONNRESET from a socket that should've been kept-alive is a good idea I think, I doubt that Django is the only web server that has made or will make this mistake. Also, major browsers appear to do this, and Cypress is supposed to behave like a normal browser.

---

In the meantime, switching to the latest Django should fix this issue. If switching is too much right now, you could try sending Connection: close with every response from your Django server, so that it follows HTTP/1.1. This should allow you to use Cypress with pages that redirect.

[x-yuri]

Good point about browsers doing this. I can confirm this about Chrome:

To be more precise it seems to open 2 connections simultaneously. Then, when the first one closes it retries the request in the second one.

And yes, Django seems to have had a bug, that was fixed in 2.1.4. I've just confirmed it. With Django 2.1.4 I can see multiple requests over the course of one connection:

The workaround for older versions is to add the following middleware:

myapp/middleware.py:

import wsgiref.util

from django.conf import settings
from  django.core.exceptions import MiddlewareNotUsed

# or else it triggers AssertionError: Hop-by-hop headers not allowed
# https://stackoverflow.com/a/57113570/52499
wsgiref.util._hoppish = {
    'keep-alive':1, 'proxy-authenticate':1,
    'proxy-authorization':1, 'te':1, 'trailers':1, 'transfer-encoding':1,
    'upgrade':1
}.__contains__

class NoKeepAliveMiddleware:
    def __init__(self, get_response):
        if not settings.DEBUG:
            raise MiddlewareNotUsed()
        self.get_response = get_response

    def __call__(self, request):
        response = self.get_response(request)
        response['Connection'] = 'close'
        return response

myapp/settings.py:

...
MIDDLEWARE = [
    ...
    'myapp.middleware.NoKeepAliveMiddleware',
    ...
]
...

Do note, that generally, you want to put it before django.middleware.common.CommonMiddleware, since the latter "performs URL rewriting based on the APPEND_SLASH and PREPEND_WWW settings." If put after CommonMiddleware, responses to e.g. /admin won't have Connection: close header added.

Also, there's probably not a bug, but behavior that could be improved. Ideally in request package. Retries are supposedly not mandatory. So, nodejs's http package seems to be too low-level for that. On the other hand, Cypress can rely on request package to do the retries. Since all other users of the request package can benefit from this.

P.S. To be honest I tried to reproduce the behavior with a freshly created Django 2.0.2 project, and failed. The trick is to add from time import sleep; sleep(1) or something after the following line. That is, it's imperative for both actions (closing the connection and sending the second request) to be performed at the same time.

[brian-mann]

@x-yuri we handle all of the low level socket layer retrying manually ourselves (as does browsers). There's no way to generically implement it in the request package because there are significant contextually necessary decisions that have to be made.

We are able to make them when we are context aware (such as a cy.visit) where we have different rules than say regular requests because those are not context aware. We replicated the rules of Chrome (but altered them in several cases) to reproduce the natural retrying behavior of browsers when we are not context aware. This was all done in the 3.3.x releases - and if you're curious we have a large number of e2e tests that demonstrate what those rules and behaviors are.

[flotwig]

Regardless, retrying one time on an ECONNRESET from a socket that should've been kept-alive is a good idea I think, I doubt that Django is the only web server that has made or will make this mistake. Also, major browsers appear to do this, and Cypress is supposed to behave like a normal browser.

Cypress will retry on an ECONNRESET: #4015

I believe that was the only action item here, so closing for now.