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