Listen for more socket failure events

[varju]

Fixes #162.

I'm not sure if there's a better way to solve the problem, but listening for the close event seems to deal with the problem for me. If the host disappears suddenly, error doesn't seem to be called. I added disconnect just in case, but perhaps this is unnecessary.

[alevy]

@varju Thanks for the PR! I've added a commit that simplifies the chages to just use the close event, which should capture both errors and non-error closes. (disconnect is not currently an event emitted from net.Socket). Can you confirm this still fixes the problem in your scenario from #162?

[varju]

Hi @alevy. Running my test case from #162 with your patch in place, and restarting the memcached container triggers an application crash with:

node:events:504
      throw er; // Unhandled 'error' event
      ^

Error: connect ECONNREFUSED 192.168.1.9:11211
    at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1157:16)
Emitted 'error' event on Socket instance at:
    at emitErrorNT (node:internal/streams/destroy:164:8)
    at emitErrorCloseNT (node:internal/streams/destroy:129:3)
    at processTicksAndRejections (node:internal/process/task_queues:83:21) {
  errno: -61,
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '192.168.1.9',
  port: 11211
}

Node.js v17.5.0

As opposed to my previous attempt at fixing this, which reported several errors like the following, and then resumed when the server recovered:

MemJS: Server <pluto.varju.ca:11211> failed after (2) retries with error - connect ECONNREFUSED 192.168.1.9:11211
Loop failed Error: connect ECONNREFUSED 192.168.1.9:11211
    at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1157:16) {
  errno: -61,
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '192.168.1.9',
  port: 11211
}

It's possible my example code lacks appropriate error handling. I confess that to some extent I'm button mashing when it comes to Node.

[saschat]

Additionally, I don't think the on close handler takes an error argument. We should probably separate the error and close handler. In the error handler just propagate the error and in the close handler deal with reconnection timeouts and so on. We just have to be careful since the error handling logic also touches reconnection variables (see line 63-67, are they even needed?).

[olemstrom]

@saschat @alevy (as you two seem to be part of MemCachier org and can probably provide us with an update here):

Is memjs still maintained? Is there any possibility of getting this PR merged. I'm more than happy to clean it up if needed, in case @varju is unresponsive after the PR having been open for such a long time.

Services I'm currently working on are facing some issues with memjs, when the connection between memcached and our application is closed on Node 15+. When writing to the socket no error is emitted (<= node 14 does emit an error) by the net.Socket. As a result, memjs does not clear the connection information and does not re-establish the connection on the next write.

Listening socket.on('close') and clearing the connection there fixes the problems we are facing so getting this PR merged would be magnificent!

I'm more than happy to provide a reproducible test case if needed.

[saschat]

@olemstrom We semi-actively maintain it. This means we fix critical bugs and review PRs, but we currently don't have the capacity to actively work on it.

As for this PR, it is broken in its current form, it actually introduces a subtle bug. As per my comment above, close and error need to be dealt with separately. A cleanup or a new PR is absolutely welcome.

[olemstrom]

@saschat Thanks for getting back to me! I have opened up a separate PR at https://github.com/memcachier/memjs/pull/170

[saschat]

Obsolete since #170