Hi Chris and Yasumasa, Sorry to have remained this issue for a long time. I come back.
I updated my patch for the following three reasons. http://cr.openjdk.java.net/~ykubota/8169358/webrev.02/ * applies to the latest jdk/jdk instead of jdk9/dev * adds test by modifying reproducer as like tests on test/jdk/com/sun/net/httpserver * prevents potential connection leak as Yasumasa said. For example, a user sets own implementation of the thread pool to HttpServer and then throws unexpected exceptions and errors. I think that we should save existing exception handler to keep compatibility and minimize the risk of connection leak by catching Throwable. I've tested test/jdk/com/sun/net/httpserver and passed. I'm not a committer, so I can not access March 5. Could you review and sponsor it? Thanks, Yuji 2016-11-11 12:11 GMT+09:00 KUBOTA Yuji <kubota.y...@gmail.com>: > Hi Chris and Yasumasa, > > Thank you for your comments! > > I had faced connection leak on production by this handler. It seems > like a corner-case but it's a real risk to me. > I had focused REE on this issue, but it is a subclass of > RuntimeException, so I think that we should investigate > RuntimeException, too. > > If Chris agrees with the covering RuntimeException by JDK-8169358, I > will investigate it and update my patch. > If we should investigate on another issue, I want to add a test case > to check fixing about REE like existing jtreg, and I will create a new > issue after an investigation about RuntimeException. > > Any thoughts? > > Thank you! > Yuji > > 2016-11-11 0:56 GMT+09:00 Chris Hegarty <chris.hega...@oracle.com>: >> >>> On 10 Nov 2016, at 14:43, Yasumasa Suenaga <yasue...@gmail.com> wrote: >>> >>> Hi, >>> >>>> I think it best to just handle the specific case of REE, as it done in >>>> Yuji’s patch. >>> >>> Will it be a cause of connection leak if RuntimeException is occurred in >>> handler method? >> >> No, at least not from this point in the code. >> >>> I think we should catch RuntimeException at least. >> >> Possibly, but it seems like a corner-case of a corner-case. >> >>>>> I think you can use finally statement to close the connection in this >>>>> case. >>>> >>>> I don’t believe that this is possible. The handling of the connection may >>>> be >>>> done in a separate thread, some time after execute() returns. >>> >>> So I said we need to check the implementation of HTTP connection and >>> dispatcher. >> >> To me, this goes beyond the scope of the issue describe in JIRA, but if >> you want to investigate that, then that’s fine. >> >> -Chris. >> >>> >>> Yasumasa >>> >>> >>> On 2016/11/10 23:00, Chris Hegarty wrote: >>>> >>>>> On 9 Nov 2016, at 12:38, Yasumasa Suenaga <yasue...@gmail.com> wrote: >>>>> >>>>> Hi Yuji, >>>>> >>>>>> http://cr.openjdk.java.net/~ykubota/8169358/webrev.01/ >>>> >>>> I think Yuji’s patch is good as is. >>>> >>>>> I think you can use finally statement to close the connection in this >>>>> case. >>>> >>>> I don’t believe that this is possible. The handling of the connection may >>>> be >>>> done in a separate thread, some time after execute() returns. I think it >>>> best >>>> to just handle the specific case of REE, as it done in Yuji’s patch. >>>> >>>>> Your patch cannot close the connection when any other runtime exceptions >>>>> are occurred. >>>>> >>>>> However, it is concerned double close if custom handler which is >>>>> implemented by the user throws runtime exception after closing the >>>>> connection. >>>>> IMHO, you need to check the implementation of HTTP connection and >>>>> dispatcher. >>>> >>>> -Chris. >>>> >>>>> >>>>> Thanks, >>>>> >>>>> Yasumasa >>>>> >>>>> >>>>> On 2016/11/08 18:53, KUBOTA Yuji wrote: >>>>>> Hi Chris, >>>>>> >>>>>> Thank you for your review and updating this issues on JBS. >>>>>> >>>>>> I filed an issue: >>>>>> https://bugs.openjdk.java.net/browse/JDK-8169358 >>>>>> >>>>>> I don't assign to me because I'm not a committer. >>>>>> >>>>>> 2016-11-08 0:28 GMT+09:00 Chris Hegarty <chris.hegarty at oracle.com>: >>>>>>>> * patch >>>>>>>> diff --git >>>>>>>> a/src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java >>>>>>>> b/src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java >>>>>>>> --- >>>>>>>> a/src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java >>>>>>>> +++ >>>>>>>> b/src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java >>>>>>>> @@ -442,10 +442,13 @@ >>>>>>>> logger.log (Level.TRACE, "Dispatcher (4)", e1); >>>>>>>> closeConnection(conn); >>>>>>>> } catch (IOException e) { >>>>>>>> logger.log (Level.TRACE, "Dispatcher (5)", e); >>>>>>>> closeConnection(conn); >>>>>>>> + } catch (RejectedExecutionException e) { >>>>>>>> + logger.log (Level.TRACE, "Dispatcher (9)", e); >>>>>>>> + closeConnection(conn); >>>>>>>> } >>>>>>>> } >>>>>>>> } >>>>>>>> _ >>>>>>>> static boolean debug = ServerConfig.debugEnabled (); >>>>>>> >>>>>>> >>>>>>> This looks ok. I wonder if some of these exceptions could be refactored >>>>>>> into a catch clause with several exception types? >>>>>> >>>>>> Yes, I agree to keep simple. I updated my patch as below. >>>>>> http://cr.openjdk.java.net/~ykubota/8169358/webrev.01/ >>>>>> Could you review it again? >>>>>> >>>>>> Thank you, >>>>>> Yuji >>>>>>> >>>>>>> -Chris. >>>>>>> >>>>>>> >>>>>>> >>>>>>>> * steps to reproduce >>>>>>>> 1. java -Djava.util.logging.config.file=logging.properties >>>>>>>> SmallHttpServer >>>>>>>> 2. post tcp connections by curl or other ways >>>>>>>> e.g.: while true; do curl -XPOST --noproxy 127.0.0.1 >>>>>>>> http://127.0.0.1:8080/; done >>>>>>>> 3. wait RejectedExecutionException occurs as below and then >>>>>>>> SmallHttpServer stops by this issue. >>>>>>>> ---- >>>>>>>> Nov 05, 2016 12:01:48 PM sun.net.httpserver.ServerImpl$Dispatcher run >>>>>>>> FINER: Dispatcher (7) >>>>>>>> java.util.concurrent.RejectedExecutionException: Task >>>>>>>> sun.net.httpserver.ServerImpl$Exchange at 37b50d9e rejected from >>>>>>>> java.util.concurrent.ThreadPoolExecutor at 1b3178d4[Running, pool size >>>>>>>> = >>>>>>>> 1, active threads = 0, queued tasks = 0, completed tasks = 7168] >>>>>>>> at >>>>>>>> java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(java.base/ThreadPoolExecutor.java:2076) >>>>>>>> at >>>>>>>> java.util.concurrent.ThreadPoolExecutor.reject(java.base/ThreadPoolExecutor.java:842) >>>>>>>> at >>>>>>>> java.util.concurrent.ThreadPoolExecutor.execute(java.base/ThreadPoolExecutor.java:1388) >>>>>>>> at >>>>>>>> sun.net.httpserver.ServerImpl$Dispatcher.handle(jdk.httpserver/ServerImpl.java:440) >>>>>>>> at >>>>>>>> sun.net.httpserver.ServerImpl$Dispatcher.run(jdk.httpserver/ServerImpl.java:405) >>>>>>>> at java.lang.Thread.run(java.base/Thread.java:844) >>>>>>>> (SmallHttpServer is stopping by not closing socket) >>>>>>>> ---- >>>>>>>> >>>>>>>> *logging.properties >>>>>>>> handlers = java.util.logging.ConsoleHandler >>>>>>>> com.sun.net.httpserver.level = FINEST >>>>>>>> java.util.logging.ConsoleHandler.level = FINEST >>>>>>>> java.util.logging.ConsoleHandler.formatter = >>>>>>>> java.util.logging.SimpleFormatter >>>>>>>> >>>>>>>> * SmallHttpServer.java >>>>>>>> import com.sun.net.httpserver.HttpExchange; >>>>>>>> import com.sun.net.httpserver.HttpHandler; >>>>>>>> import com.sun.net.httpserver.HttpServer; >>>>>>>> >>>>>>>> import java.net.InetSocketAddress; >>>>>>>> import java.util.Scanner; >>>>>>>> import java.util.concurrent.LinkedBlockingQueue; >>>>>>>> import java.util.concurrent.ThreadPoolExecutor; >>>>>>>> import java.util.concurrent.TimeUnit; >>>>>>>> >>>>>>>> public class SmallHttpServer { >>>>>>>> >>>>>>>> public static void main(String[] args) throws Exception { >>>>>>>> int POOL_SIZE = 1; >>>>>>>> String HOST = args.length < 1 ? "127.0.0.1" : args[0]; >>>>>>>> int PORT = args.length < 2 ? 8080 : Integer.valueOf(args[1]); >>>>>>>> >>>>>>>> // Setup a minimum thread pool to rise >>>>>>>> RejectExecutionException in httpserver >>>>>>>> ThreadPoolExecutor miniHttpPoolExecutor >>>>>>>> = new ThreadPoolExecutor(POOL_SIZE, POOL_SIZE, 0L, >>>>>>>> TimeUnit.MICROSECONDS, >>>>>>>> new LinkedBlockingQueue<>(1), (Runnable r) -> { >>>>>>>> return new Thread(r); >>>>>>>> }); >>>>>>>> HttpServer httpServer = HttpServer.create(new >>>>>>>> InetSocketAddress(HOST, PORT), 0); >>>>>>>> httpServer.setExecutor(miniHttpPoolExecutor); >>>>>>>> >>>>>>>> HttpHandler res200handler = (HttpExchange exchange) -> { >>>>>>>> exchange.sendResponseHeaders(200, 0); >>>>>>>> exchange.close(); >>>>>>>> }; >>>>>>>> httpServer.createContext("/", res200handler); >>>>>>>> >>>>>>>> httpServer.start(); >>>>>>>> // Wait stdin to exit >>>>>>>> Scanner in = new Scanner(System.in); >>>>>>>> while (in.hasNext()) { >>>>>>>> System.out.println(in.nextLine()); >>>>>>>> } >>>>>>>> httpServer.stop(0); >>>>>>>> miniHttpPoolExecutor.shutdownNow(); >>>>>>>> } >>>>>>>> } >>>>>>>> >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Yuji >>>>>>>> >>>>>>> >>>>>> >>>> >>
