-
Notifications
You must be signed in to change notification settings - Fork 141
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
add graceful shutdown #604
Conversation
|
f91dcc5
to
1ebb284
Compare
Conflicts: packages/node/src/app/analytics-node.ts
a9d3b27
to
2035675
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think this looks good - just had some minor questions.
}) | ||
} | ||
|
||
function sleep(timeoutInMs: number): Promise<void> { | ||
export function sleep(timeoutInMs: number): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, didn't realize we had sleep
defined here too. In the browser package we had it in lib/sleep.ts
as well so wouldn't need it defined it its version of this core file like we do currently 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we define sleep everywhere!
I figured we would make core have the canonical version and remove the other ones — thoughts? Or maybe we create a utils package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me!
timeout, | ||
}: { | ||
/** Set a maximum time permitted to wait before resolving. Default = no maximum. */ | ||
timeout?: number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of scope for this feature, but it'd be awesome if we could also tear down any pending tasks (setTimeouts and promises) that would otherwise prevent a process from exiting when we reach this timeout. (Bigger task since that means supporting something like abortSignal throughout our codebase)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, but not sure if this is needed, have you dealt with this in similar libs?
Thinking this through: If we do force resolve and there are stalled HTTP Requests to the analytics API, they wouldn't be connected to promises whose resolution was blocking an end-user network request, since our lib is very "fire-and-forget" --- apps send events passively/asynchronously and their APIs don't wait for our response before serving their users. Like a logging library.
Like, If there is a SIGTERM and something in their network or ours had gone wrong, I'm not sure what the practical difference is between terminating a stalled request to our analytics HTTP API via abort vs allowing node to do it during process.exit() (if we're talking about a client, it would be "Socket hang up" vs "The socket connection was aborted.").
(AFAIK, typical pending async callbacks in the task queue like those registered in setTimeout have no effect on server.close).
Maybe we can talk about this offline if we want to get into the weeds? I created an example app.
private _dispatch(segmentEvent: CoreSegmentEvent, callback?: Callback) { | ||
if (this._isClosed) { | ||
return undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to surface an error to the user when this happens letting them know they tried sending events after closing perhaps similar to how we emit delivery failures today? (Though I don't think you have the context at this point so not sure how difficult that would be)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I think we could emit an event or an event error with a code like "called_after_closing" here. That way, a consumer could log them or even store them in a DB for future rehydration.
3378c84
to
e695bf8
Compare
7fc6fb2
to
bb72c45
Compare
64fab5d
to
87a0ff2
Compare
87a0ff2
to
8d59e3f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me,
Graceful Shutdown
Avoid losing events on exit!
.closeAndFlush()
to stop collecting new events and flush all existing events.Graceful Shutdown: Advanced Example
https://github.com/segmentio/analytics-next/blob/9d021e2f3ea78099dd12ec50622483f945267250/packages/node/README.md