Community
Participate
Working Groups
Build Identifier: When running Jetty embedded it is useful to have a mechanism for shutting down and restarting an existing process. This is especially useful when demonstrating Jetty and when running Jetty from inside an IDE. I've currently created my own ShutdownHandler for every project where I use Jetty. This used by executing a request to the same port with a secret "cookie". Only shutdown requests from localhost should be honored. Here is an example of such a ShutdownHandler: https://github.com/jhannes/larm-poc/blob/master/larm-core-container/src/main/java/no/statnett/larm/core/container/ShutdownHandler.java Usage: https://github.com/jhannes/larm-poc/blob/master/larm-poc-server/src/main/java/no/statnett/larm/poc/LarmPocServer.java#L32 and https://github.com/jhannes/larm-poc/blob/master/larm-poc-server/src/main/java/no/statnett/larm/poc/LarmPocServer.java#L45 This would make it easier to run Jetty in embedded mode, especially when someone is just getting started Reproducible: Always
Do you want to contribute such a handler? If so please add it as an attachment and we should be able to put it into the next release. cheers
Created attachment 203728 [details] ShutdownHandlerTest A test file for the proposed change. Uses Mockito to verify the shutdown attempt
Created attachment 203729 [details] ShutdownHandler A proposed ShutdownHandler
(In reply to comment #1) > Do you want to contribute such a handler? If so please add it as an attachment > and we should be able to put it into the next release. > > cheers I've attached a proposed handler. The comments in the code shows the usage. BTW: Adding handlers to a server programmatically got clumsier in Jetty 6. What gives?
Created attachment 203899 [details] proposed patch Thx for the contribution Johannes. I prepared and attached a patch for the Handler. I left out the Unit Test as we don't have a CQ for mockito, yet. We've opened the CQ now as Mockito is pretty useful and we only need it with scope test.
my thoughts on this patch.. I don't like the System.exit() as default behavior after the server.stop() call. IMO the default behavior ought to be simply the call to stop, but I would be fine with a configuration that would enable that call, just not by default. embedded usage of this would probably want jetty shutdown but allow any other shutdown not tied to jetty to occur and the call to exit is a bit brutal. second, I am not sure why there is an attemptShutdownFromClient static call in the handler...it looks more like sample code in which can it could exist in javadoc or on the wiki where this handler ought to be *gasp* documented :) resolving these will also allow the unit test to be brought it, perhaps without mockito required (suspect this was to avoid the exit()?) regardless, mockito is approved for our usage now.
(In reply to comment #6) > my thoughts on this patch.. > > I don't like the System.exit() as default behavior after the server.stop() > call. IMO the default behavior ought to be simply the call to stop, but I > would be fine with a configuration that would enable that call, just not by > default. embedded usage of this would probably want jetty shutdown but allow > any other shutdown not tied to jetty to occur and the call to exit is a bit > brutal. > I agree. The System.exit() should be removed. > > second, I am not sure why there is an attemptShutdownFromClient static call in > the handler...it looks more like sample code in which can it could exist in > javadoc or on the wiki where this handler ought to be *gasp* documented :) > I don't agree. This code is clunky enough that it's worth encapsulating it. Putting it in ShutdownHandler is a slight violation of SRP, but the handler and shutting down will happen together. The Javadoc included with the handler documents the usage, including attemptShutdownFromClient. This example code will be repeated by anyone who wants to create an embedded server with Jetty. It would be substantially clunkier without attemptShutdownFromClient. The reason this is clunky is mostly because java.net.HttpURLConnection is clunky. If the JavaDoc included a simple code example with Jetty's HTTP client, that would be okay. But it would require the client to have more deps... > > resolving these will also allow the unit test to be brought it, perhaps without > mockito required (suspect this was to avoid the exit()?) regardless, mockito > is approved for our usage now. Mockito is there both to avoid the exit (doNothing().when(shutdownHandler).shutdownServer()) and to verify the behavior. An alternate implementation without Mockito would be to subclass ShutdownHandler in the test.
Created attachment 204364 [details] proposed patch Hi Jesse, please find attached a new version of the patch. Most important changes to the previous version: - attemptShutdownFromClient method removed - Unit Test readded as we've got Mockito CQed (note that Server.java can't be mocked as the stop() method is final) - System.exit() call optional and disabled by default Cheers, Thomas
(In reply to comment #8) > > please find attached a new version of the patch. Most important changes to the > previous version: > > - attemptShutdownFromClient method removed > If the attemptShutdownFromClient method is removed, the JavaDoc for ShutdownHandler needs to document how to actually shut down the server. Essentially the method body of attemptShutdownFromClient. (Including the try/catch) Sadly, because of the state of the Java HTTP api, this is very clumsy, so it's important that the code documents it when the attemptShutdownFromClient method is removed.
Added the javadoc for it...still been kicking other ways to do that in code but can't really make a dependency on the client here...and by itself putting in an example exchange in the client seems a bit off as well between the test and this javadoc that is probably enough for now...if I think of another way to have it codified I'll add it later
