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

Fix #173 - Queue Shutdown #217

Closed
wants to merge 6 commits into from
Closed

Conversation

izhuravlev
Copy link
Contributor

@izhuravlev izhuravlev commented Nov 15, 2019

This PR fixes issue #173.

I would like to explain what I have done.
In the code example, provided in issue #112 by @humphd, JobQueue is a class, and terminate() function is implemented as a member of this class - hence, it can be called from every place where JobQueue is used.
The main aim of the terminate() function is to close a number of queues, which are implemented in the JobQueue class. Implementing the same logic for us requires me to rewrite the structure of the feed-queue.js, making it a class.

Another thing that I wanted to mention:
I have made a little research on the "Killing" the process, and, as stated here:

"There is no way you can execute any code in your application when it is being Killed by Operating System or User. That is why it's called Killing."

The close() function for the graceful shutdown, as I understood, is just a destructor for the queue, and can be called in the other files like feed-worker.js when there is no more need for the Queue to be used.

Although, for the sake of error handling, logging and graceful termination I added the try-catch block with the logging on the feed-worker.js file.

I am open to any suggestions and will be happy to correct my work if I am wrong.

@izhuravlev
Copy link
Contributor Author

Just mentioned the stupidest "123" commit - I am sorry for this, this was done by VS code functionality which never worked for me before.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 11-29 aren't part of your PR, they shouldn't be in here. You should be working against current master.

The general pattern of what you've done here re: error handling and shut down looks sane, though.

@izhuravlev
Copy link
Contributor Author

izhuravlev commented Nov 15, 2019

@humphd how do I fix that?

Also, is what I found right and have I made what you mentioned?

@humphd
Copy link
Contributor

humphd commented Nov 15, 2019

Since there is so little code in this PR, I would suggest you do this:

git checkout master
git pull upstream master
git checkout -b old-issue-173 issue-173
git checkout -B issue-173 master
...re-do your changes in the correct files...
git add files...
git commit -m "Fix #173: add proper shutdown code for queue"
git push origin issue-173 -f

@izhuravlev
Copy link
Contributor Author

@humphd Sure do

@humphd humphd changed the title Issue 173 Fix - Queue Shutdown Fix #173 - Queue Shutdown Nov 15, 2019
@izhuravlev izhuravlev force-pushed the issue-173 branch 2 times, most recently from c2811d0 to 505c392 Compare November 15, 2019 19:17
birtony
birtony previously approved these changes Nov 15, 2019
@izhuravlev izhuravlev requested review from zufishanali, Reza-Rajabi and humphd and removed request for zufishanali and Reza-Rajabi November 15, 2019 19:32
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment. Also, the package-lock.json file is unrelated, don't include it.

git checkout -- package-lock.json

}
});
// for the sake of error handling and gracefull shutdown
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think wrapping this much code in a try catch is the right move. We'd be better to do this in something like https://nodejs.org/api/process.html#process_event_uncaughtexception

@Reza-Rajabi
Copy link
Contributor

@izhuravlev I did a small research. Here you might find the steps that you wanna take:

           Handle process kill signal
           Stop new requests from client
           Close all data process
           Exit from process

and here is an example by code. Notice how they defined the shutdown function. Also we do need to take care of both SIGTERM and SIGINT like in the mentioned example. Here is why. So you can add something like:

// from stackoverflow: 
// https://stackoverflow.com/questions/43003870/how-do-i-shut-down-my-express-server-gracefully-when-its-process-is-killed
process.on('SIGTERM', shutDown);
process.on('SIGINT', shutDown);

function shutDown() {
    console.log('Received kill signal, shutting down gracefully');
    
    // We also need to close our Redis queue
    feedQueue.close();

    // continue from stackoverflow
    server.close(() => {
        console.log('Closed out remaining connections');
        process.exit(0);
    });

    setTimeout(() => {
        console.error('Could not close connections in time, forcefully shutting down');
        process.exit(1);
    }, 10000);
}

This way we make sure any undesired interruption gets handled.
I think you can add this pice in anywhere outside the function that you currently wrapped in try {} catch {}, but I am not sure if this module is the best candidate to have this code on.
I didn't try this code and I just tried to provide something that might be useful for the last minutes.

Copy link
Contributor

@Reza-Rajabi Reza-Rajabi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have commented in Conversation

@izhuravlev
Copy link
Contributor Author

Thank you very much @Reza-Rajabi for your research!

@izhuravlev
Copy link
Contributor Author

In this commit, I have returned feed-worker.js to its initial state, created a shutDown function in the index.js, provided SIGTERM and SIGINT support inside the fs.readFile function and added logging.

@izhuravlev izhuravlev requested review from humphd and removed request for zufishanali and birtony November 19, 2019 17:10
src/index.js Outdated
@@ -5,9 +5,34 @@
const fs = require('fs');
const feedQueue = require('./feed-queue');
const feedWorker = require('./feed-worker');
const parentLogger = require('../src/lib/logger');

const log = parentLogger.child({ module: 'module-name' });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'module-name' needs to be a name. In this case, you could use something like 'main'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

src/index.js Outdated
@@ -46,6 +71,9 @@ fs.readFile('feeds.txt', 'utf8', (err, lines) => {
return;
}

process.on('SIGTERM', shutDown);
process.on('SIGINT', shutDown);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move these out of this function, to like 36 above.

@humphd humphd requested a review from miggs125 November 20, 2019 02:44
Copy link
Contributor

@Reza-Rajabi Reza-Rajabi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After applying the changes that have been requested so far, it looks very well to me.

@miggs125
Copy link
Contributor

Looks good aside from the changes @humphd requested.

@humphd
Copy link
Contributor

humphd commented Nov 23, 2019

How is this coming? Didn't see you on Thursday to find out, we should try and get this landed soon.

@izhuravlev
Copy link
Contributor Author

Wanted to rebase my fixing commit on the commit ae756fd, but I don't think that it worked out as it should.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great. A couple things I noticed and commented on.

src/index.js Outdated
}

process.on('SIGTERM', shutDown);
process.on('SIGINT', shutDown);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just move all the "shutdown" logic in here. In src/backend/web/server.js we have:

process.on('unhandledRejection', err => {
  console.log('UNHANDLED REJECTION:  Shutting down...');
  console.log(err.name, err.message);
  server.close(() => {
    process.exit(1);
  });
});

Should we move that block of code here? Then it's all contained in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks for me as process.on('unhandledRejection',... is more related to any case of error handling (like, for all of the errors), while SIGINT and SIGTERM are more related to Shutdown - so they could stay separated.

But I believe none of the logic will be broken if we move it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could export shutdown function and call it from other modules like:

process.on('unhandledRejection', importedModule.shutDown(error));

that requires slightly updating the shutDown to log the error ...

src/index.js Outdated
log.info('Received kill signal, shutting down gracefully');

// Close the Redis Queue
feedQueue.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably await this, or see if you can pass a callback. At the very least, we should log errors that this produces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@humphd Will using await feedQueue.close() be enough?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's so tricky. If we want to do it await, maybe we want also setTimeout before await feedQueue.close();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my suggestion:

setTimeout() => {
    log.error('Could not close connections in time, forcefully shutting down');
    process.exit(1);
}, 10000);

// Try to shut down 
Promise.all([
  // Shutdown the queue
  feedQueue.close().catch(err => log.error('Unable to close feed queue gracefully', err),
  // Shutdown the web server - we could also use util.promisify if you want
  new Promise((resolve, reject) => server.close(err => {
    if(err) {
      log.error('Unable to close web server gracefully', err);
      reject(err);
    } else {
      resolve();
    }
   })
])
.then(() => process.exit(0))
.catch(err => log.error(error));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this while looking up something for your code: https://expressjs.com/en/advanced/healthcheck-graceful-shutdown.html.

We could do one of those in a later PR too. I don't want to expand this PR forever, but it's neat to see.

@humphd
Copy link
Contributor

humphd commented Nov 28, 2019

I've tested this locally, made a few tweaks, and merged this into master. I wanted to make sure it got in so we didn't push it back yet again. Thanks for working on it.

@humphd humphd closed this Nov 28, 2019
Main automation moved this from In progress/Review to Closed Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Main
Closed
Development

Successfully merging this pull request may close these issues.

None yet

6 participants