Bug 357687

Comment 1 Greg Wilkins 2011-09-20 00:12:40 EDT

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

Comment 2 Johannes Brodwall 2011-09-21 03:20:51 EDT

Created attachment 203728 [details]
ShutdownHandlerTest

A test file for the proposed change. Uses Mockito to verify the shutdown attempt

Comment 3 Johannes Brodwall 2011-09-21 03:21:33 EDT

Created attachment 203729 [details]
ShutdownHandler

A proposed ShutdownHandler

Comment 4 Johannes Brodwall 2011-09-21 03:22:28 EDT

(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?

Comment 5 Thomas Becker 2011-09-23 09:38:38 EDT

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.

Comment 6 Jesse McConnell 2011-09-28 12:24:39 EDT

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.

Comment 7 Johannes Brodwall 2011-09-29 08:28:09 EDT

(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.

Comment 8 Thomas Becker 2011-09-30 06:08:09 EDT

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

Comment 9 Johannes Brodwall 2011-10-03 11:56:02 EDT

(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.

Comment 10 Jesse McConnell 2011-10-06 17:03:02 EDT

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