mafintosh / turbo-http

Blazing fast low level http server
MIT License
1k stars 47 forks source link

ECONNRESET when handling requests asynchronously #12

Closed pietermees closed 6 years ago

pietermees commented 6 years ago

I'm seeing an issue where if

  1. the request handler is asynchronous (in that it doesn't respond to the request in the same event loop tick)
  2. the server receives new requests while a previous one is still being handled

I will randomly get an ECONNRESET error on the client without any finish, end, close or error events on the server req.socket object.

mafintosh commented 6 years ago

Can you share an example?

pietermees commented 6 years ago

I'll see if I can bring it down to a small reproduction

On Tue, Mar 13, 2018 at 7:34 PM Mathias Buus notifications@github.com wrote:

Can you share an example?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mafintosh/turbo-http/issues/12#issuecomment-372772688, or mute the thread https://github.com/notifications/unsubscribe-auth/AG3lJTVtqcPQuJunKN5ITl6vC38re7nvks5teBEqgaJpZM4SpNn3 .

santoshrajan commented 6 years ago

Looks like same problem for me too. Browser requests 6 files simultaneously. Get this random server error

Error: Not writable
    at Connection._notWritable (/Users/santosh/workspace/turbo-http/node_modules/turbo-net/lib/connection.js:246:49)
    at Connection._write (/Users/santosh/workspace/turbo-http/node_modules/turbo-net/lib/connection.js:189:37)
    at Connection.write (/Users/santosh/workspace/turbo-http/node_modules/turbo-net/lib/connection.js:164:15)
timdp commented 6 years ago

@santoshrajan Do you mean that the error to the write() callback is supposed to be translated to an error event? As I understand it, @pietermees isn't getting an error at all in his server code, so if the stack trace that you posted is related, something is supposed to translate the callback argument to an 'error' event, right?

santoshrajan commented 6 years ago

He probably did not get the error, bcos he called write without a callback? In which the server does not show anything.

pietermees commented 6 years ago

Correct, I'm calling res.end with some data and no callback. I assumed that if an error would occur in this case it would have been raised as an error event on either the request, connection or server objects?

timdp commented 6 years ago

Yeah, I thought that was the behavior on the native net and http implementations.

pietermees commented 6 years ago

Here's a minimal reproduction of the issue. Two notes:

  1. I'm not getting an error callback on the server side
  2. It's not tied to a specific number of requests, with REQUESTS = 16 I can occasionally reproduce it
const turbo = require('turbo-http')
const http = require('http')

const REQUESTS = 64

const server = turbo.createServer(function (req, res) {
  const data = Buffer.from('abcdef'.repeat(1000))
  res.setHeader('Content-Length', data.length)
  res.end(data, (err) => {
    if (err != null) {
      console.warn('Server Error:', err)
    }
  })
})

const fireRequests = async () => {
  const promises = []
  for (let i = 0; i < REQUESTS; ++i) {
    promises.push(new Promise((resolve, reject) => {
      const req = http.get({ hostname: 'localhost', port: 8080, path: '/' }, (res) => {
        res.resume()
        resolve()
      })
      req.on('error', reject)
    }))
  }
  try {
    await Promise.all(promises)
  } catch (err) {
    console.warn(`Client Error:`, err)
  } finally {
    server.close()
  }
}

server.listen(8080, fireRequests)
mafintosh commented 6 years ago

@pietermees hmm just ran that and i got no errors in general. am i doing something run? what node version are you on btw? and are you sure the dependencies are up to date?

pietermees commented 6 years ago

Node version:

$ node -v
v9.8.0

I just wrote up that test in a completely new folder with no previously installed dependencies, so they're fresh.

Maybe if you increase the number of simultaneous requests?

Maybe also relevant: I'm running OSX 10.13.3

aks- commented 6 years ago

@pietermees it seems issue is in http module and not turbo-http. i could reproduce this issue after replacing the turbo-http with http server.

pietermees commented 6 years ago

With some further digging I think I found the origin of the issue.

TLDR: the server runs out of backlog for new connections

It's indeed possible to reproduce the issue with Node's native http module, but only with much larger request counts.

The reason is the backlog parameter to server.listen. Node uses 511 by default, although this is further limited by the OS's SOMAXCONN setting. On my Mac that defaults to 128. If I set SOMAXCONN=1024 I can get rid of the error.

The reason why the problem happens much sooner with turbo-http is that turbo-net sets the backlog size to a fixed 5. This means that only 5 connections can be queued up to be accepted by the server before new connections get rejected. By sending a lot of simultaneous requests, it's easy to cross that limit.

Here's the line in turbo-net causing this (note the TODO about researching backlog): https://github.com/mafintosh/turbo-net/blob/master/src/turbo_net.c#L190

mafintosh commented 6 years ago

Should be fixed in latest now. @pietermees you wanna confirm and close the issue?

pietermees commented 6 years ago

@mafintosh retested with the new release and confirmed that https://github.com/mafintosh/turbo-net/pull/9 resolved the issue!

santoshrajan commented 6 years ago

@pietermees Good job! Been after this for a few days with no luck!