Clear socket on MemJS.Server when connection is closed

[olemstrom]

Some services I'm working on have been facing issues with MemJS on Node 15+. This seems to be related to the fact that net.Socket does not emit "error" when memjs attempts to write into an already closed socket.

This PR adds socket.on('close') listener to clear up socket on MemJS.Server. After the change, memjs will automatically re-establish the connection on next read / write to memcached even on Node 15+.

Unfortunately, I could not figure how to reproduce the issue in memjs test suite as I needed to bring in a real memcached server with me. Here is a script demonstrating the bug we are facing:

// Insert into ./test/e2e_test.js
// Run using `node ./test/e2e_test.js`

var MemJS = require("../");

console.log("Initialising Memcached connection");
// Create a real server as the behaviour is not reproducible without making a real connection to the server
var server = new MemJS.Server("localhost", "11211");
var client = new MemJS.Client([server]);

console.log("Write initial value with a live connection");
client.set("foo", "bar").then(function () {
  console.log("Close connection (can happen for a myriad of other reasons too");
  server.close();

  // Waiting is required for some reason to
  // reproduce the failure case we are seeing on Node 15+
  // Something to do with the event loop maybe?
  console.log("Waiting for 1s...");
  setTimeout(function () {
    client
      .set("foo", "baz")
      .then(function (success) {
        // This will not run without the patch applied on Node 15+
        // "Error: This socket has been ended by the other party" will be logged on Node <= 14
        // but write will succeed anyway (new socket is created)
        console.log("Succesfully updated:", success);
        client.quit();
      })
      .catch(function (err) {
        // We will never reach this catch clause
        console.error(err);
      });
  }, 1000);
});

[saschat]

Thanks for the PR. While this seems to address your issue, it introduces new potential bugs. Let's say an error occurs and the socket error event is triggered. This will trigger the error handler which changes the server state:

self.connected = false;  // changes server state
if (self.timeoutSet) {
  self._socket.setTimeout(0);
  self.timeoutSet = false;  // changes server state
}
self._socket = undefined;  // changes server state
self.error(error);  // changes server state

After the error event, a close event is triggered (see https://nodejs.org/api/net.html#event-error_1). This will change the server state again in the same manner (except for the error call at the end). This is fine if both events are handled immediately one after the other. But what happens if the Node scheduler throws handling a new request in between?

This is what I meant by "separating the error and close handlers" in #163. In the error handler, we should only do error-handling stuff and move all the rest to the close handler.

Personally, I would remove lines 153 to 158. Technically, some of the stuff in the Server.error function on line 57 should also happen on close instead of error. However, I am reluctant to touch those since it is called on multiple occasions and I don't know the implications of changing this might have.

@alevy thoughts?

[saschat]

@olemstrom Sorry for the delay. I talked to @alevy and he agrees with my assessment. Can you remove 153 to 158 and make sure your issue is still solved. Then the PR should be good to be merged.

[olemstrom]

@saschat Thanks for your insight! No worries about the delay, I was not available last week anyway and would not have been able to address your comment 😄

I have removed the socket clearing code (lines 153 - 158) from the .on('error') handler.

[saschat]

@olemstrom v1.3.1 is now published.