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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): improve states and add shutdown hooks for applications #4145

Merged
merged 10 commits into from Dec 3, 2019

Conversation

@raymondfeng
Copy link
Member

raymondfeng commented Nov 18, 2019

Inspired by https://github.com/godaddy/terminus

The PR improves the application lifecycle as follows:

  1. Allow graceful shutdown when the application is running inside a managed container, such as Kubernetes Pods. The container sends signals such as SIGTERM to notify the application so that it has a chance to close resources before it's terminated after a grace period. See https://cloud.google.com/blog/products/gcp/kubernetes-best-practices-terminating-with-grace.

  2. Tidy up the check for application states to leave no surprise to app.start and app.stop calls if they are invoked out of sequence.

Docs: https://github.com/strongloop/loopback-next/blob/add-shutdown-hooks/docs/site/Life-cycle.md

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

@raymondfeng raymondfeng requested a review from bajtos as a code owner Nov 18, 2019
@raymondfeng raymondfeng force-pushed the add-shutdown-hooks branch 7 times, most recently from ad3c0c8 to 0db5415 Nov 18, 2019
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Nov 19, 2019

@raymondfeng can you please beef up the pull request description and explain what new feature(s) you are adding, which use cases you are trying to address and how are the new features addressing them?

There is very little documentation in your code changes. Please find a way how to document the new behavior for users of our framework, we don't want them to grok the code or look up this pull request to understand your additions.

Copy link
Member

bajtos left a comment

鈽濓笍

@raymondfeng raymondfeng force-pushed the add-shutdown-hooks branch from 0db5415 to e3a873e Nov 19, 2019
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Nov 19, 2019

@bajtos PR description updated. I'll add some docs too.

@raymondfeng raymondfeng force-pushed the add-shutdown-hooks branch 2 times, most recently from d369116 to 8d455ef Nov 19, 2019
@raymondfeng

This comment has been minimized.

});

function normalizeStdoutData(output: string[]) {
// The received events can be `['start\n', 'stop\nstopped\n']` instead

This comment has been minimized.

Copy link
@emonddr

emonddr Nov 19, 2019

Contributor

What is the difference in these two lists in the comments?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Nov 19, 2019

Author Member

The array has different number of items.

This comment has been minimized.

Copy link
@emonddr

emonddr Nov 22, 2019

Contributor

The array has different number of items.

array 1: ['start\n', 'stop\nstopped\n'] has two items: 'start\n' and 'stop\nstopped'
array2: [ 'start\n', 'stop\n', 'stopped\n' ] has three items: 'start'\n, 'stop\n', 'stopped\n'

why does the second item in the first array contain 2 items separated by \n ? Is this a typo?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Nov 22, 2019

Author Member

No, that's how Node.js 8.x writes data to the stream - it ends up with one data event instead of two (in Node 10.x and above).

expect(sig).to.eql(expectedSignal);
}
// The received events can be `['start\n', 'stop\nstopped\n']` instead
// of [ 'start\n', 'stop\n', 'stopped\n' ]

This comment has been minimized.

Copy link
@emonddr

emonddr Nov 19, 2019

Contributor

What is the difference in these two lists in the comments?

@emonddr emonddr self-requested a review Nov 19, 2019
@raymondfeng raymondfeng requested a review from bajtos Nov 21, 2019
- started -> started (no-op)

The `start` throws an error if it's called with an invalid application state,
such as `starting` and `stopping`.

This comment has been minimized.

Copy link
@bajtos

bajtos Nov 22, 2019

Member

Should we allow start as a no-op while the application is in a starting state?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Nov 22, 2019

Author Member

IMO, no. Consider the following code:

await app.start(); // The promise should be not resolved until the application is started
// We assume the app is now in `started` state

The other option is to have app.start() wait for started event while it's starting.

This comment has been minimized.

Copy link
@bajtos

bajtos Nov 28, 2019

Member

The other option is to have app.start() wait for started event while it's starting.

That's what I would expect from app.start() method. As a developer calling app.start, I want to ensure the app is running. I don't necessarily care if it's stopped or started, I want to get it to the started state.

In my experience, APIs that offer idempotence are easier to use.

- stopped -> stopped (no-op)

The `stop` throws an error if it's called with an invalid application state,
such as `starting` and `stopping`.

This comment has been minimized.

Copy link
@bajtos

bajtos Nov 22, 2019

Member

Similarly here, I think stop should be allowed while stopping.

@@ -36,6 +36,7 @@ describe('application metadata booter acceptance tests', () => {
app = new MyApp({
rest: givenHttpServerConfig(),
});
await app.start();

This comment has been minimized.

Copy link
@bajtos

bajtos Nov 22, 2019

Member

I am confused. Why do we need to start the app? I don't see any change in the tests above. Is this need for a change a signal that you are introducing a breaking change, i.e. existing LB4 projects can break after upgrading to a newer version of LB4 runtime?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Nov 22, 2019

Author Member

We can revert this.

it('allows application.stop when it is created', async () => {
const app = new Application();
await app.stop(); // no-op
expect(app.state).to.equal('created');

This comment has been minimized.

Copy link
@bajtos

bajtos Nov 22, 2019

Member

I find this confusing. How comes that after calling app.stop, the app can be either in created or stopped state? I would expect it to be always in stopped state. By allowing two different states, we are pushing more complexity on our users.

Maybe this is a hint that instead of having two states created and stopped, we should have a single state only, e.g. not-running?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Nov 22, 2019

Author Member

Good question. We may have a few choices:

  1. Do not allow stop
  2. Transition from created to stopped
  3. Merge the state

This comment has been minimized.

Copy link
@bajtos

bajtos Nov 28, 2019

Member

Originally, I wanted to propose to merge the two states and use stopped instead.

However, now that you have introduced booting & booted states, this is becoming even more confusing. Can we call app.start() before the application was booted? Or does it require the app to be in the state booted? Can we "un-boot" the application, i.e. go from booted back to created? I think that's not possible and not feasible to implement either.

So IIUC, we have the following states with the following allowed transitions:

  1. created - can call app.boot() to go to booting
  2. booting - no transition allowed, boot in progress, will go to booted
  3. booted - app is booted but not started yet. Call app.start() to go to starting state
  4. starting - no transition allowed, will go to started
  5. started - can call app.stop() to go to stopping state
  6. stopping - no transition allowed, will go to stopped
  7. stopped - can call app.start() to go to starting state

If the list above is correct, then I am proposing to remove stopped state and move the app to booted state after it has stopped.

Thoughts?

/cc @strongloop/loopback-next

*/
public async start(): Promise<void> {
this.assertValidStates('start', 'created', 'stopped', 'started');

This comment has been minimized.

Copy link
@bajtos

bajtos Nov 22, 2019

Member

Should we define named constants for these magic strings? Preferably as a string enum, see https://www.typescriptlang.org/docs/handbook/enums.html#string-enums.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Nov 22, 2019

Author Member

I'm considering so. On the other hand, do we allow other states, such as booting and booted?

const kill = () => {
// eslint-disable-next-line @typescript-eslint/no-misused-promises
signals.forEach(sig => process.removeListener(sig, cleanup));
process.kill(process.pid, signal);

This comment has been minimized.

Copy link
@bajtos

bajtos Nov 22, 2019

Member

IIUC, this code is trying to kill the process by sending it the same signal that we have already intercepted. Is this the right usage of process.on feature? I mean aren't we kind of creating an infinite loop, where the signal handler send the same signal again?

@sam-github would you mind lending us our expertise? We are trying to intercept signals like KILL so that we can perform application-specific cleanup before exiting the process.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Nov 22, 2019

Author Member

No, it's intentional. The first kill is trapped to run stop as the _isShuttingDown flag is false. The 2nd kill proceeds with routine Node.js default logic to shut down the process.

});

function normalizeStdoutData(output: string[]) {
// The received events can be `['start\n', 'stop\nstopped\n']` instead

This comment has been minimized.

Copy link
@emonddr

emonddr Nov 22, 2019

Contributor

The array has different number of items.

array 1: ['start\n', 'stop\nstopped\n'] has two items: 'start\n' and 'stop\nstopped'
array2: [ 'start\n', 'stop\n', 'stopped\n' ] has three items: 'start'\n, 'stop\n', 'stopped\n'

why does the second item in the first array contain 2 items separated by \n ? Is this a typo?

@raymondfeng raymondfeng force-pushed the add-shutdown-hooks branch from 8d455ef to 00679a4 Nov 22, 2019
@raymondfeng raymondfeng force-pushed the add-shutdown-hooks branch 3 times, most recently from 5b1d23a to 09e8eef Nov 22, 2019
@raymondfeng raymondfeng requested a review from bajtos Nov 25, 2019
it('allows application.stop when it is created', async () => {
const app = new Application();
await app.stop(); // no-op
expect(app.state).to.equal('created');

This comment has been minimized.

Copy link
@bajtos

bajtos Nov 28, 2019

Member

Originally, I wanted to propose to merge the two states and use stopped instead.

However, now that you have introduced booting & booted states, this is becoming even more confusing. Can we call app.start() before the application was booted? Or does it require the app to be in the state booted? Can we "un-boot" the application, i.e. go from booted back to created? I think that's not possible and not feasible to implement either.

So IIUC, we have the following states with the following allowed transitions:

  1. created - can call app.boot() to go to booting
  2. booting - no transition allowed, boot in progress, will go to booted
  3. booted - app is booted but not started yet. Call app.start() to go to starting state
  4. starting - no transition allowed, will go to started
  5. started - can call app.stop() to go to stopping state
  6. stopping - no transition allowed, will go to stopped
  7. stopped - can call app.start() to go to starting state

If the list above is correct, then I am proposing to remove stopped state and move the app to booted state after it has stopped.

Thoughts?

/cc @strongloop/loopback-next

@bajtos bajtos added the feature label Nov 29, 2019
@raymondfeng raymondfeng force-pushed the add-shutdown-hooks branch from 09e8eef to 29190aa Nov 29, 2019
@raymondfeng raymondfeng requested a review from bajtos Dec 2, 2019
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Dec 2, 2019

@bajtos PTAL.

@raymondfeng raymondfeng force-pushed the add-shutdown-hooks branch from 29190aa to d978515 Dec 2, 2019
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Dec 2, 2019

application states

@raymondfeng raymondfeng force-pushed the add-shutdown-hooks branch from d978515 to 80b30d1 Dec 3, 2019
@bajtos
bajtos approved these changes Dec 3, 2019
Copy link
Member

bajtos left a comment

I love the diagram you added, it makes it so much easier to understand the states and the transitions between them 馃憦

I still feel we have too many states, which makes it more difficult for our users to understand how to write robust state observers, it's easy to accidentally forget to listen on all necessary events.

At the same time I'd like to move forward, I guess we will have to wait for user feedback and see how our LB developers are going to use this new feature (if at all).

@raymondfeng raymondfeng merged commit da9a7e7 into master Dec 3, 2019
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.01%) to 92.175%
Details
@raymondfeng raymondfeng deleted the add-shutdown-hooks branch Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.