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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log as info exceptions from server after sending stop with StopMojo. #9188

Merged
merged 3 commits into from Jan 25, 2023

Conversation

janbartel
Copy link
Contributor

Closes #9129

Catch and log at info level any exceptions from server after jetty StopMojo sends stop command.

@janbartel janbartel self-assigned this Jan 17, 2023
sbordet
sbordet previously approved these changes Jan 19, 2023
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would catch (Throwable) like we do elsewhere, but besides that LGTM.

@gsf-sellis
Copy link

gsf-sellis commented Jan 19, 2023

I might go the other direction and only catch (SocketException), since we know it is expected and harmless. Anything else is unexpected and maybe should be propagated up the stack?

@janbartel
Copy link
Contributor Author

I might go the other direction and only catch (SocketException), since we know it is expected and harmless. Anything else is unexpected and maybe should be propagated up the stack?

@gsf-sellis there's not much the plugin can do if there's any kind of exception thrown after we've sent the forcestop command. Depending on the configuration, the plugin will either exit straight away after sending the forcestop command, or - if we have been able to obtain the pid of the forked child ` - we will wait for the process to exit.

@gsf-sellis
Copy link

gsf-sellis commented Jan 24, 2023

I'm just thinking in terms of keeping the catch block specific to the case that we know is harmless and that we can recover from. Anything else would be uncaught, propagated up the stack, and logged at a higher level than info (like the SocketException was before this PR). That said, however you all want to implement it is fine with me. Thanks for addressing the issue! I look forward to having clean logs again. :-)

@gsf-sellis
Copy link

gsf-sellis commented Jan 24, 2023

I look forward to having clean logs again.

Oops I'm realizing now that info is the default log level, so that means this error will still appear in the logs?

I was hoping to maybe hide this error at the info level, since it is not really an error and makes the logs messy.

One option might be to catch SocketException and log it at debug or trace?

@janbartel
Copy link
Contributor Author

I look forward to having clean logs again.

Oops I'm realizing now that info is the default log level, so that means this error will still appear in the logs?

I was hoping to maybe hide this error at the info level, since it is not really an error and makes the logs messy.

One option might be to catch SocketException and log it at debug or trace?

How about this: if debug is enabled we output the full stacktrace, otherwise we output an info with just the message?

@gsf-sellis
Copy link

That sounds great, thanks!

if (getLog().isDebugEnabled())
getLog().error("Error after sending command: " + command + ". Check the server state.", e);
else
getLog().info("Error after sending command: " + command + ". Check the server state.");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "Error" could be e.getMessage()? It's probably SocketException: Connection reset but could be something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, how about the latest commit then?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! It's too bad we have to work around Windows TCP issues like this but it will be a quality of life improvement for me and others. Thanks!

@janbartel janbartel merged commit d46f6f7 into jetty-10.0.x Jan 25, 2023
@janbartel janbartel deleted the jetty-10.0.x-9129-rst-on-stop-mojo branch January 25, 2023 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jetty-maven-plugin JettyStopMojo intermittently errors with SocketException: Connection reset
3 participants