fix server listen error event many times

[pagewang0]

No description provided.

[Gerhut]

I reviewed the codebase and found that the unit test did not cover the server error but only the socket error, and the server error was not handled correctly. Resolved in de20de8

Could you rebase your PR and see if the test cases are still working correctly?

[pagewang0]

I reviewed the codebase and found that the unit test did not cover the server error but only the socket error, and the server error was not handled correctly. Resolved in de20de8

Could you rebase your PR and see if the test cases are still working correctly?

Yes, the test cases are working with no problems!

[coveralls]

coverage: 100.0%. remained the same
when pulling 54969d2 on pagewang0:master
into 511eef4 on Gerhut:master.

[Gerhut]

Does it setup error handler only if there is already one?

[pagewang0]

No, because the server listens for errors using on, not once. You can try emitting the error many times.

[Gerhut]

Why does it remove?

[pagewang0]

Because this code can fix the server's repeated listening problem.

if (server.eventNames().includes('error')) {
    server.on('error', reject)
  }

[Gerhut]

Can you add a teat case to describe your issue?

[pagewang0]

That is my error, I mean code is this

if (!server.eventNames().includes('error')) {
    server.on('error', reject)
}

I will write test case to describe my issue.

[pagewang0]

my test case: https://github.com/pagewang0/axiosist/blob/master/test.js#L197 @Gerhut

[Gerhut]

No, the error listener is not supposed to be kept here, which is causing memory leak.

[Gerhut]

Neither

[Gerhut]

This is not testing axiosist but testing node.js's http module

[Gerhut]

This doesn't make sense - only the first request will listen to the server error, others will hang if server emits error event.

[pagewang0]

I tried running your off error code. Can you run this test case?:

test('should be listened error to once', async t => {
  const server = createServer()

  server.on('request', (req, res) => {
    res.end('ok')
  })

  server.listen()

  const request = axiosist(server)
  const promises = []

  for (let i = 0; i < 20; i++) {
    promises.push(() => request.get('/'))
  }

  await Promise.all(promises.map(promise => promise()))

  t.is(server.listenerCount('error'), 0)
})

The repeated listening error still exists. If an error is emitted, then many listeners will receive the error message because your off operation is asynchronous.

[Gerhut]

Consider:

test('should be listened error to once', async t => {
  const request = axiosist((req, res) => {
    res.end('ok')
  })
  const promises = []

  for (let i = 0; i < 20; i++) {
    promises.push(() => request.get('/'))
  }

  await Promise.all(promises.map(promise => promise()))
})

This does not seem a typical unit test, if you need concurrent requests to the same server with such number, consider stress tests like autocannon.

[pagewang0]

Maybe you are right. I just want to prove the error listening issue.

[Gerhut]

I am thinking about maybe we can just drop this event listener - serious server errors will be transferred to the socket, which is already handled by axios (one example: https://github.com/axios/axios/blob/v1.7.2/lib/adapters/http.js#L571-L574).

[pagewang0]

Usually, when the server encounters an error, it is handled by a try-catch block or its own listener, so I think this error listener can be removed too.