After Response is ending (res.end()), still receive the data from client (res.onData)

[trylovetom]

Expected Behavior

When res.end is is executed, res.onData event will not be emit.

Actual Behavior

I sent the sample image (3.9mb), after res.end is is executed, res.onData still emit and receive the data from client.

server

% node index.js
limit 1048576
current size: 65328 bytes isAborted: false isLast: false
current size: 589616 bytes isAborted: false isLast: false
current size: 1113904 bytes isAborted: false isLast: false
current size: 1511620 bytes isAborted: true isLast: false
current size: 2035908 bytes isAborted: true isLast: false
current size: 2560196 bytes isAborted: true isLast: false
current size: 3084484 bytes isAborted: true isLast: false
current size: 3474944 bytes isAborted: true isLast: false
current size: 3999232 bytes isAborted: true isLast: false
current size: 4106429 bytes isAborted: true isLast: true

client

% http post localhost:3000 < sample.jpg
HTTP/1.1 413 Payload Too Large
Content-Length: 0
uWebSockets: v0.17

How to Reproduce

httpie command

http post localhost:3000 < sample.png

my code

const uWS = require('uWebSockets.js')

const app = uWS.App()

app.any('/*', res => {
  const limit = 1 * 1024 * 1024 // 1 mega bytes
  console.log('limit', limit)

  let data = Buffer.allocUnsafe(0)
  let isAborted = false

  res.onData((chunk, isLast) => {
    data = Buffer.concat([data, Buffer.from(chunk)])

    console.log('current size:', data.length, 'bytes isAborted:', isAborted, 'isLast:', isLast)

    if (isAborted) {
      return
    }

    if (data.length > limit) {
      res.writeStatus('413 Payload Too Large')
      res.end()
      isAborted = true
      return
    }

    if (isLast) {
      res.writeStatus('202 Accepted')
      res.end()
      isAborted = true
    }
  })

  res.onAborted(() => {
    isAborted = true
    console.log('aborted')
  })
})
app.listen('0.0.0.0', 3000, () => {})

smaple.jpg

Environment

• Node.js v14.2.0
• uWebSockets.js v17.4.0
• httpie v2.1.0
• macOS (Catalina v10.15.4)
  • MacBook Pro (Retina, 13-inch, Early 2015)
  • 2.7 GHz Core x2
  • 8 GB DDR3 Ram x1

[ghost]

You should only respond when isLast is set. Http does not allow the server to refuse a big payload, you still have to receive it (but you can ignore it). That's as far as I know at least.

[trylovetom]

According rfc7231 The server MAY close the connection to prevent the client from continuing the request.

Such as Nginx, go gin Close the connection, when a request is over the limit

So I can send the many TB size payload to block the bandwidth and fill the memory.

[mikeandtherest]

What if you use res.close() instead? That one should "Immediately force closes the connection." as per doc.

If that doesn't work either (for whatever reason, since it should), just move data = Buffer.concat([data, Buffer.from(chunk)]) down a few lines, after the isAborted check. That should do it as a workaround...

[trylovetom]

@mikeandtherest If use res.close() instead of res.end(), it will be close immediately. But the client side will not get any response like status code 413 Payload Too Large.

[ghost]

You can res.close in process.nextTick but that's a hacky solution. This feature is being tracked in the main repo.

[ghost]

I have thought about it and the most reliable and simple solution I can think of is to add res.endAndClose() as an alternative to res.end() where once drained, the connection is shutdown. You still have to ignore any further data you might get above your limit.

You can't really make good heuristics such as "if res.end() is called before seeing all data then make it res.endAndClose()" because some users simply ignore the data and in that case it would be really bad to close down the connection rather than keep it keep-alive. And in cases where you would like to stream back whatever data you got, it becomes messy.

I think the simplest solution is res.endAndClose(). The name is debatable - endWithClose?

[ghost]

Another solution is to have a second argument like res.end("data", shutdown true/false);

[mikeandtherest]

You can't really make good heuristics such as "if res.end() is called before seeing all data then make it res.endAndClose()"

I totally agree that no assumptions should be made, since they could indeed start as good intentions but can end up as restrictions and make the whole thing less flexible.

I think the simplest solution is res.endAndClose(). The name is debatable - endWithClose? Another solution is to have a second argument like res.end("data", shutdown true/false);

Tbh I would look at it from the opposite angle, and by that I mean that what I think it's missing is a "close with a response/payload". So maybe res.closeWithResponse() or res.closeWithData() could be better?

[ghost]

Those are good names but they can be difficult to connect with "end" which is really the same thing. I ended up making it res.end("data", true) for closeConnection since I did not really want to add more functions than needed. This is committed and working in main repo now, so next js release should have it supported.

[mikeandtherest]

Ok, as long as the default value for the new arg is false (which I see it's already the case in your commit), I think we're all good since it fixes the problem while also keeping it safe for existing code. Thx!