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/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 >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>> >>>>> >>
