reverse the order of instance.Stop() and onShutdown() calls on Restart

[rdrozhdzh]

1. What does this change do, exactly?

reverse the order of instance.Stop() and onShutdown() calls on Restart. See more details at coredns/coredns#1666 (comment)

2. Please link to the relevant issues.

coredns/coredns#1666

3. Which documentation changes (if any) need to be made because of this PR?

none

4. Checklist

• I have written tests and verified that they fail without my change
• I have squashed any insignificant commits
• This change has comments for package types, values, functions, and non-obvious lines of code
• I am willing to help maintain this change if there are issues with it later

[CLAassistant]

All committers have signed the CLA.

[mholt]

Thanks; just checking on the error handling.

[mholt]

If the old instance fails to stop... shouldn't we return the error, rather than pretending that everything is continuing nominally?

[fturib]

You may want to return the errors due to stop the old instance.
But as the new instance is started correctly, most likely the caller will need to make

So I guess, we would like to:
1- try to run all shutdown and stop (whatever the errors we get here ...) - we can collect all errors
2- finish the start (emit event InstanceStartupEvent),
3- return the new instances and the list of errors from stopping.

Then let the caller decide if it needs to completely stop all instances to restart properly, or keep running whatever invalid stop of old instance.

But doing so, the return statement will be ambiguus between + Error or + Errors.

[rdrozhdzh]

Yes, there is no simple way of error handling in case if we have successfully started new instance and could not stop the old instance.

In original implementation, the loop of calling onShutdown() handlers broke in case if some handler returned error. I think this is not correct. onShutdown() handlers may be used to release some resources the plugins allocated in onStartup(). So, we should try to call all onShutdown() handlers even if some other handlers failed

We could return the errors collected during the stop, but what will the caller do with this list? Print to log? We already do this here.

Another reason of returning no error is in current logic of error processing on caller side - see the code below
https://github.com/mholt/caddy/blob/f7757da7edceb23de39e2460fd59068eb5834c93/sigtrap_posix.go#L86-L100

Here we restore old event hooks in case if Restart() returns error. This makes sense in case if we was not able to start new instance. However, in case if we have started new instance successfully but was not able to stop the old instance, this logic doesn't make sense. If we have fully functional new instance (i.e. the start completed without error, and perhaps new hooks might be added by on plugin) and the old instance in unpredictable (likely unhealthy) state (because it returned error) I would prefer keeping new hooks. And that's why Restart() in my fix returns no error if stopping the old instance failed.

@mholt, what do you think the most appropriate error handling in this case? Please, let me know which change should I make here. Or I can revert to original errors handling

[rdrozhdzh]

Well, I'm going to simplify the fix. I will keep only Stop/onShutdown reordering and will revert error processing related stuff. The error processing could be improved later in separate PR if needed

[mholt]

Looks like #2262 will be merged soon; might be worth checking if that conflicts with this.

Edit: I'm all for small PRs, so thanks! 👍

[francislavoie]

@mholt there's no conflict with #2262 (the changes there are all in lines higher up in the file), this looks good to go

[mholt]

Great, thanks everyone!