adonisjs / ace

Node.js framework for creating command line applications
MIT License
361 stars 36 forks source link

Cleanup before shutdown initiated by SIGINT / SIGTERM in ace command #151

Closed stropitek closed 3 months ago

stropitek commented 1 year ago

I'd like to run custom async code when an ace command receives SIGTERM / SIGINT signal. Does a lifecycle method exist which allows to do that before the shutdown methods are called?

My use case is to run some database queries when the command receives those signals in order not to leave the database in an intermediate state.

I already tried doing that by listening to SIGTERM on the process myself. But the database connections got closed while the cleanup was running so that didn't work. I'd like to avoid if possible to have to recreate database connections myself in order to achieve that.

thetutlage commented 1 year ago

Is it inside an AdonisJS app? Also, can you please share your command's code along with the SIGTERM listener you created?

stropitek commented 1 year ago

Is it inside an AdonisJS app?

Yes it's running in an AdonisJS app.

I changed my command to do sort of a minimal repro (see below). I use the https://github.com/zakodium/adonis-mongodb provider, but I suppose it could also be reproduced with a lucid provider.

Note that when sending SIGINT, AdonisJS does not shut down so I dont run into the issue. I would have to exit the process myself (which I don't do here). I suppose AdonisJS is not doing anything with SIGINT.

When sending SIGTERM though, I have an error coming from the mongodb provider Client must be connected before running operations.

import assert from 'node:assert';

import { BaseCommand } from '@adonisjs/core/build/standalone';

import Database from '@ioc:Zakodium/Mongodb/Database';

export default class ImportNext extends BaseCommand {
  public static override commandName = 'import:next';
  public static override description = 'Import the next pending scan';

  public static override settings = {
    loadApp: true,
  };

  public override async run() {
    const logger = this.logger;
    const connection = Database.connection('mongodb');
    const scansCollection = await connection.collection('scans');

    const result = await scansCollection.findOneAndUpdate(
      { status: { $in: ['PENDING', 'PENDING_REIMPORT'] } },
      { $set: { status: 'IMPORTING' } },
      {
        projection: {
          _id: 1,
          status: 1,
        },
      },
    );

    if (result.value === null) {
      this.logger.info('nothing to run');
      return;
    }

    function handler() {
      logger.info('cleaning up');
      // my original handler contains some more logic to go back to the initial state
      // I added another wait to simulate me actually doing multiple async operations
      wait(10)
        .then(() => {
          assert(result.value !== null);
          return scansCollection.findOneAndUpdate(
            { _id: result.value._id },
            { $set: { status: result.value.status } },
          );
        })

        .then(() => {
          logger.success('cleaned up');
        })
        .catch((error) => {
          // Results in an error because the connection is closed
          logger.error(error.message);
        });
    }

    // Handle unexpected termination
    addSignalListeners(handler);

    // We do some image processing stuff that takes about a minute to execute
    // I replaced it with a wait function
    logger.info('waiting 60 seconds');
    await wait(60_000);

    removeSignalListeners(handler);

    // Update the status to imported
    await scansCollection.findOneAndUpdate(
      { _id: result.value._id },
      { $set: { status: 'IMPORTED' } },
    );
  }
}

const signals: NodeJS.Signals[] = ['SIGINT', 'SIGTERM'];
function addSignalListeners(handler: (signal: NodeJS.Signals) => void) {
  for (const signal of signals) {
    process.addListener(signal, handler);
  }
}

function removeSignalListeners(handler: (signal: NodeJS.Signals) => void) {
  for (const signal of signals) {
    process.removeListener(signal, handler);
  }
}

function wait(ms: number) {
  // eslint-disable-next-line no-promise-executor-return
  return new Promise((resolve) => setTimeout(resolve, ms));
}
thetutlage commented 1 year ago

I am not in front of computer right now, but can you try hooking into the onExit handler?

https://github.com/adonisjs/ace/blob/72b8020d770625009319b972fcb74e0d6ca56364/src/Kernel/index.ts#L411

The code might roughly look like

this.kernel.onExit(asyncCallback)
stropitek commented 1 year ago

If I add a line to register this onExit callback in my example above, the callback is never called when sending a SIGTERM

targos commented 1 year ago

I'm thinking about providing some mechanism with AbortController, but while this is a good way to signal the command that it has to stop what it's doing, I'm not sure then how it can tell the framework that cleanup is done.

targos commented 1 year ago

@RomainLanz's https://github.com/RomainLanz/adonis-bull-queue probably suffers from this issue as well. The job processing has to be stopped (and eventually let jobs gracefully handle a stop mid-flight) before killing the process.

stale[bot] commented 9 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

targos commented 6 months ago

@thetutlage Could you please reopen?

thetutlage commented 6 months ago

I have re-opened it. But I think this should not be an issue with v6. For now, I am happy to publish a patch if you can send a PR for the fix.

thetutlage commented 3 months ago

Going to close the issue, as I think with v6 this should not be an issue and for v5 I will prefer a PR