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

Close immediately after requests are done #16

Closed
ErisDS opened this issue Aug 9, 2020 · 11 comments · Fixed by #25 or sraka1/http-terminator#1
Closed

Close immediately after requests are done #16

ErisDS opened this issue Aug 9, 2020 · 11 comments · Fixed by #25 or sraka1/http-terminator#1
Labels
enhancement New feature or request released

Comments

@ErisDS
Copy link

ErisDS commented Aug 9, 2020

In the readme it states that http-terminator should wait for requests to finish before closing connections.

As far as I can tell, if http-terminator is set to have a long timeout, and has to handle a long-running request. It actually doesn't close until the timeout ends, even if all the requests have finished.

Reproduce:

  • Setup a little express app with 2 endpoints:
app.get('/', (req, res) => res.send('Hello World!'));
app.get('/long', (req, res) => {
    setTimeout(function () {
        res.status(200).send('Done long request');
    }, 10000);
});
  • Make sure to start your server something like this:
 setInterval(() => this.server.getConnections(
         (err, connections) => console.log(`${connections} connections currently open`)
), 3000);

 this.httpTerminator = createHttpTerminator({
     server: this.server,
     gracefulTerminationTimeout: 60000
});

process.on('SIGINT', function () {
     this.httpTerminator.terminate();
}

Now to test:

  • Run your app.
  • make a request to the homepage
  • Ctrl + c / send SIGINT

See the server closes immediately

  • Run your app.
  • make a request to the /long
  • Ctrl + c / send SIGINT
  • Watch the connections get closed until there is 1 left
  • Note the open request finishes after 10000ms, now there are 0 connections

See the server doesn't close for a very long time.

I would expect that the server would close as soon as the conditions that there are 0 connections and the last request is finished are met OR after 60000ms, whichever is first.

@ErisDS
Copy link
Author

ErisDS commented Aug 9, 2020

Note: I just ran the same test with stoppable, and it closes once the last request finishes. Obviously it has other issues though :)

@gajus gajus added the enhancement New feature or request label Aug 10, 2020
@gajus
Copy link
Owner

gajus commented Aug 10, 2020

Hey, thanks for reporting this – are you available to PR either a failing test case or a fix + test?

@ErisDS
Copy link
Author

ErisDS commented Aug 10, 2020

I can provide access to my demo code that shows this library and stoppable don't do the same thing later today.

I did have a look at the tests and the code to see what I could do, but it's tricky to grasp quickly.

As far as I can tell the first test here https://github.com/gajus/http-terminator/blob/master/test/helpers/createTests.js#L38 is what should be testing this case, but it doesn't include testing that the server closes. I can't see any test with a strategy for testing that the server closes?

If you can point me in the direction of the pattern/strategy you would use for that, I can probably work from there.

@ErisDS
Copy link
Author

ErisDS commented Aug 10, 2020

Also just to add - the code doesn't have much in the way of comments except "// $FlowFixMe" which I don't understand?

@ErisDS
Copy link
Author

ErisDS commented Aug 17, 2020

Should I share my demo code?

@jylauril
Copy link

I'm having the same problem with a very basic setup. Didn't have this problem with stoppable.

I tested changing my code to just call the server.close instead of the httpTerminator.terminate and that worked much better. I did notice that after enough manual hard restarts (killing the process manually), it suddenly started working for a while but went back to hanging state after X amount of restarts.

That makes this really hard to debug, so I might just switch back to stoppable to get going again.

@amit777

This comment has been minimized.

@jylauril

This comment has been minimized.

@ErisDS

This comment has been minimized.

@gajus
Copy link
Owner

gajus commented Sep 11, 2020

Thanks Eris for your input, but the conversation is derailing.

Valuable contributions at this point are:

  • a test case that is failing (not a link to your demo)
  • a test case with a fix

Beyond this, the bug has been described and acknowledged.

@gajus
Copy link
Owner

gajus commented Sep 17, 2021

🎉 This issue has been resolved in version 3.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment