httpserver does not close connections when RejectedExecutionException occurs

[Yasumasa Suenaga]

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?
I think we should catch RuntimeException at least.


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


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