> On 15 Nov 2014, at 13:59, Mark Sheppard <mark.shepp...@oracle.com> wrote: > > Hi > please oblige and review the updated patch as per Chris' suggestions > > http://cr.openjdk.java.net/~msheppar/8015692/webrev.03/
Thanks for taking the comments onboard Mark. The updated webrev looks good. -Chris. > regards > Mark >> On 13/11/2014 10:49, Chris Hegarty wrote: >>> On 12 Nov 2014, at 22:12, Mark Sheppard <mark.shepp...@oracle.com> wrote: >>> >>> Hi >>> Please oblige and review the updated patch >>> http://cr.openjdk.java.net/~msheppar/8015692/webrev.02a/ >> The source change looks fine to me. Just a comment/question; since you are >> swallowing the IE I think you should restore the interrupt status on the >> thread. >> >> try { >> dispatcherThread.join(); >> } catch (InterruptedException e) { >> Thread.currentThread().interrupt(); <<<< HERE >> logger.log(Level.FINER, "ServerImpl.stop: ", e); >> } >> >> The test could be simplified somewhat by creating one HttpServer, getting >> its port (getServerAddress()) , and then re-using that port. Maybe a little >> less #’s in the output ? ;-) >> >> -Chris. >> >>> to address the issue >>> https://bugs.openjdk.java.net/browse/JDK-8015692 >>> >>> as suggested the ServerImpl.stop() method performs a join, only, for the >>> dispatcher thread >>> so that all housekeeping is done prior to exiting stop. >>> >>> regards >>> Mark >>> >>>> On 24/02/2014 17:08, Chris Hegarty wrote: >>>> Hi Mark, >>>> >>>> I think join should be sufficient here. >>>> >>>> I understand your argument to move selector close into stop, but that just >>>> seems to require extra co-ordination between stop and the dispatcher loop, >>>> namely you now need to check if the selector is closed in a few places. I >>>> think it is simpler to leave the original code as is, dispatcher closes >>>> the selector, and only selectNow is invoked from stop. Or maybe I'm >>>> missing something. >>>> >>>> -Chris. >>>> >>>>> On 21/02/14 12:21, Mark Sheppard wrote: >>>>> Hi Chris, >>>>> thanks for the response. >>>>> >>>>> Yes, that's true. >>>>> It was just the way it evolved as I analyzed the issue. >>>>> Originally, the join was after the close and selectNow. >>>>> The close was moved from Dispatcher to stop, as there was some >>>>> "interplay" between the Dispatcher thread >>>>> and the stop thread, when the Dispatcher was invoking the close. >>>>> Then added the join() in the stop method, to ensure that the Dispatcher >>>>> wasn't still executing after the server had been >>>>> stopped. >>>>> >>>>> As the Selector is opened in the ServerImpl constructor and not in the >>>>> Dispatcher, it seemed >>>>> from a symmetry view point more logical to invoke the close in the >>>>> ServerImpl stop >>>>> >>>>> The selectNow is just insurance for cleanup purposes. >>>>> >>>>> It is possible that the join should be higher up in the stop() flow i.e. >>>>> immediately after the >>>>> setting the finish flag? >>>>> As such, the Dispatcher should be finished with the various >>>>> HttpConnection collections, before >>>>> the stop processes them. >>>>> >>>>> regards >>>>> Mark >>>>> >>>>>> On 21/02/2014 07:22, Chris Hegarty wrote: >>>>>> Mark, >>>>>> >>>>>> I agree with you, there is certainly some additional co-ordination >>>>>> needed between the thread invoking the stop method and the dispatcher >>>>>> thread. >>>>>> >>>>>> I wonder why you needed to add the selectNow() and the close() after >>>>>> you have joined the dispatcher thread? Since you are guaranteed that >>>>>> the dispatcher thread will have exited before join() returns? >>>>>> >>>>>> -Chris. >>>>>> >>>>>>> On 17 Feb 2014, at 01:20, Mark Sheppard <mark.shepp...@oracle.com> >>>>>>> wrote: >>>>>>> >>>>>>> Hi >>>>>>> >>>>>>> Please oblige and review the changes in the webrev >>>>>>> >>>>>>> http://cr.openjdk.java.net/~msheppar/8015692/jdk9/webrev/ >>>>>>> >>>>>>> to address the issue raised in the bug >>>>>>> >>>>>>> https://bugs.openjdk.java.net/browse/JDK-8015692 >>>>>>> >>>>>>> Summary: >>>>>>> a series Junit tests which start stop instances of an >>>>>>> com.sun.net.httpserver.HttpServer failed due to >>>>>>> java.net.BindException: Address already in use: bind >>>>>>> >>>>>>> This was raised against Windows XP, but the sample test to reproduce >>>>>>> the issue >>>>>>> was run on Windows 7, and the problem was seen to occur on this OS also. >>>>>>> The sample was run against jdk7, jdk8 and jdk9: reproducible on each. >>>>>>> >>>>>>> On investigation it appears that some additional co-ordination is >>>>>>> required between the >>>>>>> HttpServer's (actually SereverImpl) dispatcher thread and the thread >>>>>>> invoking the stop >>>>>>> method. This change has amended the stop method to wait for the >>>>>>> Dispatcher thread to complete, then >>>>>>> invokes the selector's selectNow, to handled cancelled events, and >>>>>>> closes the selector. >>>>>>> The selector.close() has been removed from the Dispatcher's run method. >>>>>>> >>>>>>> regards >>>>>>> Mark >
