Stop does not properly wait for the server to close

[dcherman]

Thanks for providing this lib, I was about to write something like this when I saw you mention this in the k8s slack channel :)

Was looking through the code and noticed that the stop method does not properly wait for the server to close since no callback is provided. Since the method was marked async, it seems like you had intended for it to wait.

lightship/src/factories/createLightship.js

Line 133 in c453672

server.close();

[gajus]

Thank you

[gajus]

🎉 This issue has been resolved in version 2.0.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[dcherman]

Hey @gajus, thanks for taking a look at this. If you don't mind a suggestion, I'd create a new promise that resolves when your server is closed (or fails to close I guess) rather than calling process.exit() since you can't assume that other resources that the application uses like databases, redis, etc.. have all been cleaned up yet

[gajus]

This behaviour is by design.

The idea is that Lightship registers all the shutdownHandler and forces termination after all shutdown handlers have been called.

I have added a warning about the active event loop preventing the program to gracefully exit and changed the exit code to 1.

Does this seem reasonable solution?

If not, please advise how would you handle it, since the requirement is to terminate the program within a given time limit.

[gajus]

🎉 This issue has been resolved in version 2.1.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[dcherman]

Ah, yea, that makes a lot of sense. I was thinking about this incorrectly. I think the warning is a nice touch though since that might help people find dangling resources that they missed cleaning up.

In that case, my only remaining suggestion would be to use Promise.all and map to return an array of promises from the shutdown handlers. The current behavior would call each of them in series and possibly cause shutdown to take longer than it otherwise should. Don't think we necessarily need to enforce order here since that can be accomplished in userland if necessary.

[gajus]

In that case, my only remaining suggestion would be to use Promise.all and map [..]

Cannot really do that.

Suppose that user does this.

const connection = [..];

registerShutdownHandler(async () => {
  await connection.query([..]);
});

registerShutdownHandler(async () => {
  await connection.end();
});

Using Promise.all would potentially interrupt whatever the dependent shutdown handler is doing.

[dcherman]

Sure, but do you feel that it's correct for users to depend on the ordering of shutdown handlers in your lib? It feels more correct for them to have done something like:

  registerShutdownHandler(async () => {
    try {
      await connection.query([..]);
    } finally {
      await connection.end();
    }
});

rather than depending on implementation details of this lib to determine whether or not the resource is still available

[gajus]

Fair point.

I will come back to this in a bit.

#6

Don't want to rush the change. Will check how comparable libraries implement shutdown handler execution.

[gajus]

This wouldn't be safe though:

registerShutdownHandler(async () => {
    try {
      await connection.query([..]);
    } finally {
      await connection.end();
    }
});

registerShutdownHandler(async () => {
    try {
      await connection.query([..]);
    } finally {
      await connection.end();
    }
});

[gajus]

Just do:

registerShutdownHandler(async () => {
  await Promise.all([
    userCollectionOfShutdownHandlers,
  ]);
});

if you need parallelism.