httpserver does not close connections when RejectedExecutionException occurs
KUBOTA Yuji
kubota.yuji at gmail.com
Thu Mar 1 11:40:46 UTC 2018
Hi all,
Could you please review my patch(s)?
Thanks,
Yuji
2018-02-20 14:21 GMT+09:00 KUBOTA Yuji <kubota.yuji at 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.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