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