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