Hi Christoph, Thank you for your review and comment!
2018-03-06 20:38 GMT+09:00 Langer, Christoph <christoph.lan...@sap.com>: > I had a look and to me it seems safe to do like webrev.03 suggests. That is, > remove the throws clause. As Daniel pointed out, it seems that all callers of > the "handle" method would do merely the same. The only thing I'm not sure > about is the CancelledKeyException caught in line 413. In that case no > exception would be logged currently. With your change, it would now inside > "handle". Maybe you want to handle a CancelledKeyException explicitly in > "handle" now and suppress the log? SelectionKey::isReadable might throw CancelledKeyException in line 397, so the logging in line 413 still work except the exceptions from inside "handle". I want to handle unexpected exceptions and errors from "handle" to minimize the risk of connection leak. Separating logging also would help developers. > What tests did you run? I've tested test/jdk/com/sun/net/httpserver and passed. I can not access Mach5 because I'm not a committer. > BTW: you could remove line 396: ' boolean closed;' while you are touching > this file. It is not needed. Nice catch! I removed this unused variable. http://cr.openjdk.java.net/~ykubota/8169358/webrev.04/ Best regards Yuji > Best regards > Christoph > >> -----Original Message----- >> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of >> KUBOTA Yuji >> Sent: Donnerstag, 1. März 2018 12:41 >> To: OpenJDK Network Dev list <net-dev@openjdk.java.net> >> Cc: Yasumasa Suenaga <yasue...@gmail.com> >> Subject: Re: httpserver does not close connections when >> RejectedExecutionException occurs >> >> Hi all, >> >> Could you please review my patch(s)? >> >> Thanks, >> Yuji >> >> 2018-02-20 14:21 GMT+09:00 KUBOTA Yuji <kubota.y...@gmail.com>: >> > Hi Daniel, >> > >> > Thank you for your comment. >> > >> > Dispatcher is package-private class and handle method is called at >> > only this file in the package (sun.net.httpserver), and all call sites >> > look like to handle exception suitably. So we can remove `throws >> > IOException` clause without modifying the call site logic as like >> > webrev.03. >> > http://cr.openjdk.java.net/~ykubota/8169358/webrev.03 >> > >> > However, I could not find whether can I change a signature of public >> > method without specified steps. If we need to write CSR to remove >> > `throws IOException` clause, we should remove the clause by other >> > issue. And I want to commit webrev.02 at this issue. >> > http://cr.openjdk.java.net/~ykubota/8169358/webrev.02 >> > >> > I'd like to get your thoughts on that. >> > >> > P.S. I'm an author, not a committer, so I cannot access Mach5. >> > >> > Thanks, >> > Yuji >> > >> > 2018-02-17 1:11 GMT+09:00 Daniel Fuchs <daniel.fu...@oracle.com>: >> >> Hi, >> >> >> >> On the surface I'd say that this change looks reasonable. >> >> However, handle is declared to throw IOException, and >> >> with this change it is guaranteed to never throw even >> >> RuntimeException. >> >> >> >> It seems that the code that calls handle() - at least in >> >> this file, has some logic to handle IOException - or >> >> other exception possibly thrown by Dispatcher::handle, >> >> which as far as I can see is not doing much more than what >> >> closeConnection does. So it looks as if calling >> >> closeConnection in handle() instead of throwing could be OK. >> >> >> >> However it would be good to at least try to figure out >> >> whether there are any other call sites for Dispatcher::handle, >> >> validate that no longer throwing will not modify the call site >> >> logic, and possibly investigate if the 'throws IOException' clause >> >> can be removed from Dispatcher::handle (that is: look >> >> whether Dispatcher is exposed and/or can be subclassed and >> >> if there are any existing subclasses). >> >> >> >> best regards, >> >> >> >> -- daniel >> >> >> >> >> >> On 16/02/2018 15:29, KUBOTA Yuji wrote: >> >>> >> >>> 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/ThreadPoolExecut >> or.java:842) >> >>>>>>>>>>> at >> >>>>>>>>>>> >> >>>>>>>>>>> >> java.util.concurrent.ThreadPoolExecutor.execute(java.base/ThreadPoolExec >> utor.java:1388) >> >>>>>>>>>>> at >> >>>>>>>>>>> >> >>>>>>>>>>> >> sun.net.httpserver.ServerImpl$Dispatcher.handle(jdk.httpserver/ServerImp >> l.java:440) >> >>>>>>>>>>> at >> >>>>>>>>>>> >> >>>>>>>>>>> >> sun.net.httpserver.ServerImpl$Dispatcher.run(jdk.httpserver/ServerImpl.ja >> va: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 >> >>>>>>>>>>> >> >>>>>>>>>> >> >>>>>>>>> >> >>>>>>> >> >>>>> >> >>
