Using websocket causes socket leaks in nginx

[andreymal]

I use nginx with configuration similar to this:

http {
  # ...

  map $http_upgrade $connection_upgrade {
    default  upgrade;
    ''       close;
  }

  server {
    # ...

    location /mailpit/ {
      proxy_pass http://127.0.0.1:8025;

      proxy_http_version 1.1;
      proxy_set_header Upgrade $http_upgrade;
      proxy_set_header Connection $connection_upgrade;

      proxy_set_header Host $http_host;
      proxy_set_header X-Real-IP $remote_addr;
      proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
      proxy_set_header X-Forwarded-Proto $scheme;
    }
  }
}

and after these steps:

1. Open mailpit in the browser (which creates a websocket connection);
2. Close the tab (which closes the connection);
3. Restart nginx within ~20 seconds —

I've noticed that restarting nginx is taking longer than usual. I checked /var/log/nginx/error.log and found this:

2023/11/10 17:04:37 [alert] 7854#7854: *8 open socket #26 left in connection 6
2023/11/10 17:04:37 [alert] 7854#7854: *7 open socket #25 left in connection 7
2023/11/10 17:04:37 [alert] 7854#7854: aborting

I tested several other projects that also use websockets, but could not reproduce it. Only Mailpit causes this error.

I think this is because, unlike other projects, Mailpit doesn't try to read anything from the websocket and therefore doesn't know it's already closed.

If my assumption is correct, this error could be solved by adding a loop that reads the websocket and does nothing else, something like (disclaimer — I'm not a Go expert):

for {
    _, r, err := c.conn.NextReader()
    if err != nil {
        // gracefully close the connection
        // c.conn.Close(), unregister in the hub or something
        break
    }

    // drop the websocket message
    _, err = io.Copy(io.Discard, r)
}

[axllent]

I believe you're absolutely correct @andreymal, the websockets do appear to be left open it seems (disregarding the nginx entirely). I think they time out eventually, but a proactive approach is needed. I think I've found a solution but I need to do a bit more testing first.

[axllent]

@andreymal A fix for this has just been released in v1.10.0. Please confirm this solves your issue? Thanks.

[andreymal]

@axllent yes 👍

_{(but using ReadMessage instead of NextReader allocates memory, and an evil client could kill Mailpit by sending a giant message like websocket.send(new ArrayBuffer(1024 * 1024 * 1024)), but if you don't care about evil clients then okay)}

[axllent]

Awesome, thanks for the quick feedback, and the note re: NextReader() - you're right about that (hadn't crossed my mind). I have made that change which will be included in the next release.

[axllent]

Just a FYI - the switch to NextReader() was included in the v1.10.1 release 👍