Change 'Goodbye!' message to be output after listeners are closed

[MSP-Greg]

Description

When a server is stopped, the last line of the log is Goodbye!. Currently, this line is output before the listeners are closed. Note that this is not output when a server is halted.

While working with updated tests, and checking for closed listeners, I noticed this.

Hence, change Goodbye! to appear after the listeners are closed. Whether Goodbye! should be output when a server is halted, not sure...

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
• I have added appropriate tests if this PR fixes a bug or adds a feature.
• My pull request is 100 lines added/removed or less so that it can be easily reviewed.
• If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
• If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
• I have updated the documentation accordingly.
• All new and existing tests passed, including Rubocop.

[nateberkopec]

This also gets called on hot restarts on windows and Jruby though.

Is the goal here to log this message on halts? If so, I think it should just be extracted into its own method and then called separately in both places.

[MSP-Greg]

This also gets called on hot restarts on windows and JRuby though

Using the PR, 'Goodbye!' is not logged when restarting on Windows. Haven't checked JRuby.

While working with CI POpen Puma servers, I wanted to check proper closing of bind sockets on shutdown, and I used the output of 'Goodbye!' as a marker for 'shutdown complete'. As mentioned above, 'Goodbye!' may be written before the bind sockets are closed.

Is the goal here to log this message on halts?

Not really, but I mentioned it because I thought the behavior should match. I think we could just replace if @status == :stop with if @status == :halt || @status == :stop. We could add a method, but maybe that's overkill?

[nateberkopec]

I think I'm nervous about the idea of moving a shutdown log into a method that doesn't actually shut anything down

[MSP-Greg]

Maybe move it to Events#fire_on_stopped!? I don't think that fires on 'halt'? Should it?

[nateberkopec]

The only real difference between halt and stop should be that stop waits gracefully while halt just shuts everything down ASAP, everything else should be the same. So I think logging should be consistent.

Firing events though is a bit trickier, since they kind of get in the way of the mission of halt (please shutdown immediately!). Let's leave firing that event on halt for another day.

[MSP-Greg]

I updated this, I believe it works correctly (on both halt & stop)...

[dentarg]

I think it doesn't hurt mention this change in the changelog, even if it's considered minor :)

[MSP-Greg]

@dentarg - feature, bug fix, or refactor? I'm not sure which...

[dentarg]

Bugfix sounds the best match to me

[MSP-Greg]

Bugfix sounds the best match to me

Done. Warning, merge conflict ahead...