httpserver does not close connections when RejectedExecutionException occurs
Daniel Fuchs
daniel.fuchs at oracle.com
Fri Feb 16 16:11:55 UTC 2018
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