WIP: First pass at ignoring signals #271
Conversation
| if (process.env.IGNORE_SIGNALS) { | ||
| return logger.info('Ignoring SIGINT/SIGTERM as requested.') | ||
| } |
There was a problem hiding this comment.
@lpradovera return if IGNORE_SIGNALS is set so the SDK does not attach listeners with process.on
|
I'm not a huge fan of this solution... ignoring signals solves the problem directly, but is kind of an odd solution, relying on pushing signal handling to the end user for a fairly common scenario of having to clean up or wait for call completion. On startup we have On shutdown we only have I would suggest instead we expose another callback method (name TBD) that allows the user to manage their business logic while shutting down but before its disconnected from Relay. The result I think is the same, clients would write the same code, but it would be in a SDK supported callback rather than hacking signal handling. |
|
That would be a great solution, though we are going to eventually redo all
SDKs. Also, adding a consumer interface method makes it so all SDKs have to
support it. I think this is a good stopgap measure.
…On Fri, 20 Nov 2020 at 20:12, Bryan Rite ***@***.***> wrote:
I'm not a huge fan of this solution... ignoring signals solves the problem
for Replicant directly, but is kind of an odd solution, relying on pushing
signal handling to the end user for a fairly common scenario of having to
clean up or wait for call completion.
On startup we have setup (pre-connect) and ready (post-connect)
On shutdown we only have teardown (post-disconnect)... what we're missing
is a pre-disconnect callback method.
I would suggest instead we expose another callback method (name TBD) that
allows the user to manage their business logic while shutting down but
before its disconnected from Relay.
The result I think is the same, Replicant would write the same code, but
it would be in a SDK supported callback rather than hacking signal handling.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#271 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB2TSV5PHLRGZFXQZSBRWTSQ25SXANCNFSM4T356XAA>
.
|
|
Not really, we should eventually have it for all of them, but we can add it just for Node right now, it's just as much if not less code than this... rather than having a client rely on something we won't support in the future. |
|
Ok, I understand better now. You are right. |
|
Can you take an attempt at it? Would just have to follow whatever is done for |
No description provided.