Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
stropitek opened this issue May 26, 2023 · 9 comments
Open
Assignees
Labels
Status: Abandoned Dropped and not into consideration

Comments

@stropitek
Copy link

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
Copy link
Member

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

@thetutlage thetutlage self-assigned this May 31, 2023
@stropitek
Copy link
Author

stropitek commented May 31, 2023

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
Copy link
Member

thetutlage commented Jun 1, 2023

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

public onExit(callback: (kernel: this) => void | Promise<void>): this {

The code might roughly look like

this.kernel.onExit(asyncCallback)

@stropitek
Copy link
Author

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

@targos
Copy link
Member

targos commented Jul 21, 2023

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
Copy link
Member

targos commented Jul 21, 2023

@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.

Copy link

stale bot commented Dec 15, 2023

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.

@stale stale bot added the Status: Abandoned Dropped and not into consideration label Dec 15, 2023
@stale stale bot closed this as completed Mar 17, 2024
@targos
Copy link
Member

targos commented Mar 18, 2024

@thetutlage Could you please reopen?

@thetutlage thetutlage reopened this Mar 21, 2024
@thetutlage
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Abandoned Dropped and not into consideration
Projects
None yet
Development

No branches or pull requests

3 participants