httpserver does not close connections when RejectedExecutionException occurs
KUBOTA Yuji
kubota.yuji at gmail.com
Tue Feb 20 05:21:51 UTC 2018
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.fuchs at 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.yuji at 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.hegarty at oracle.com>:
>>>>
>>>>
>>>>> On 10 Nov 2016, at 14:43, Yasumasa Suenaga <yasuenag at 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 <yasuenag at 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>
More information about the net-dev
mailing list