Re: httpserver does not close connections when RejectedExecutionException occurs

[KUBOTA Yuji]

Thank you, Daniel and everyone!

I will keep how to make the suitable changeset when next contributions.

Best regards,
Yuji

2018-03-09 1:26 GMT+09:00 Daniel Fuchs :
> Thank you Yuji!
>
> This has been pushed.
>
> For the record - since you are an author, you don't need
> to list your address in the Contributed-by section as you
> already appear as the author of the changeset:
>
> http://hg.openjdk.java.net/jdk/jdk/rev/5447851ff0f6
> ```
> author  ykubota
> ```
>
> best regards,
>
> -- daniel
>
>
> On 08/03/2018 09:28, KUBOTA Yuji wrote:
>>
>> Thank you!
>>
>> Let's use the following changeset if there aren't any problems.
>> http://cr.openjdk.java.net/~ykubota/8169358/final_changeset_jckecked
>>
>> Best regards
>> Yuji
>
>

Re: httpserver does not close connections when RejectedExecutionException occurs

[Daniel Fuchs]

Thank you Yuji!

This has been pushed.

For the record - since you are an author, you don't need
to list your address in the Contributed-by section as you
already appear as the author of the changeset:

http://hg.openjdk.java.net/jdk/jdk/rev/5447851ff0f6
```
author  ykubota
```

best regards,

-- daniel

On 08/03/2018 09:28, KUBOTA Yuji wrote:

Thank you!

Let's use the following changeset if there aren't any problems.
http://cr.openjdk.java.net/~ykubota/8169358/final_changeset_jckecked

Best regards
Yuji

Re: httpserver does not close connections when RejectedExecutionException occurs

[KUBOTA Yuji]

2018-03-08 16:47 GMT+09:00 Langer, Christoph <christoph.lan...@sap.com>:
> looks good.

Thank you!

Let's use the following changeset if there aren't any problems.
http://cr.openjdk.java.net/~ykubota/8169358/final_changeset_jckecked

Best regards
Yuji

>
> Best regards
> Christoph
>
>> -Original Message-
>> From: KUBOTA Yuji [mailto:kubota.y...@gmail.com]
>> Sent: Donnerstag, 8. März 2018 03:57
>> To: Daniel Fuchs <daniel.fu...@oracle.com>
>> Cc: Langer, Christoph <christoph.lan...@sap.com>; Yasumasa Suenaga
>> <yasue...@gmail.com>; OpenJDK Network Dev list > d...@openjdk.java.net>
>> Subject: Re: httpserver does not close connections when
>> RejectedExecutionException occurs
>>
>> Hi Daniel,
>>
>> Thank you for your sponsoring!
>>
>> 2018-03-08 0:56 GMT+09:00 Daniel Fuchs <daniel.fu...@oracle.com>:
>> > Could you prepare a final changeset with a properly formatted
>> > comment [1], use jcheck [2] to verify that your changeset conforms
>> > to the OpenJDK rules (no trailing whitespaces, no tabs etc...),
>> > and upload it somewhere to http://cr.openjdk.java.net/~ykubota/8169358
>> ?
>>
>> My final changeset is below. (no jcheck error from webrev.04)
>> http://cr.openjdk.java.net/~ykubota/8169358/final_changeset_jckecked
>>
>> If Christoph could not check webrev.04, please use the following changeset.
>> http://cr.openjdk.java.net/~ykubota/8169358/final_changeset_jckecked-
>> two-reviewers
>>
>> Best regards
>> Yuji

RE: httpserver does not close connections when RejectedExecutionException occurs

[Langer, Christoph]

Hi Yuji,

looks good.

Best regards
Christoph

> -Original Message-
> From: KUBOTA Yuji [mailto:kubota.y...@gmail.com]
> Sent: Donnerstag, 8. März 2018 03:57
> To: Daniel Fuchs <daniel.fu...@oracle.com>
> Cc: Langer, Christoph <christoph.lan...@sap.com>; Yasumasa Suenaga
> <yasue...@gmail.com>; OpenJDK Network Dev list  d...@openjdk.java.net>
> Subject: Re: httpserver does not close connections when
> RejectedExecutionException occurs
> 
> Hi Daniel,
> 
> Thank you for your sponsoring!
> 
> 2018-03-08 0:56 GMT+09:00 Daniel Fuchs <daniel.fu...@oracle.com>:
> > Could you prepare a final changeset with a properly formatted
> > comment [1], use jcheck [2] to verify that your changeset conforms
> > to the OpenJDK rules (no trailing whitespaces, no tabs etc...),
> > and upload it somewhere to http://cr.openjdk.java.net/~ykubota/8169358
> ?
> 
> My final changeset is below. (no jcheck error from webrev.04)
> http://cr.openjdk.java.net/~ykubota/8169358/final_changeset_jckecked
> 
> If Christoph could not check webrev.04, please use the following changeset.
> http://cr.openjdk.java.net/~ykubota/8169358/final_changeset_jckecked-
> two-reviewers
> 
> Best regards
> Yuji

Re: httpserver does not close connections when RejectedExecutionException occurs

[KUBOTA Yuji]

Hi Daniel,

Thank you for your sponsoring!

2018-03-08 0:56 GMT+09:00 Daniel Fuchs :
> Could you prepare a final changeset with a properly formatted
> comment [1], use jcheck [2] to verify that your changeset conforms
> to the OpenJDK rules (no trailing whitespaces, no tabs etc...),
> and upload it somewhere to http://cr.openjdk.java.net/~ykubota/8169358 ?

My final changeset is below. (no jcheck error from webrev.04)
http://cr.openjdk.java.net/~ykubota/8169358/final_changeset_jckecked

If Christoph could not check webrev.04, please use the following changeset.
http://cr.openjdk.java.net/~ykubota/8169358/final_changeset_jckecked-two-reviewers

Best regards
Yuji

Re: httpserver does not close connections when RejectedExecutionException occurs

[Chris Hegarty]

On 07/03/18 01:07, KUBOTA Yuji wrote:

..
Nice catch! I removed this unused variable.
http://cr.openjdk.java.net/~ykubota/8169358/webrev.04/

I think is good. Thanks.

-Chris.

Re: httpserver does not close connections when RejectedExecutionException occurs

[Daniel Fuchs]

Hi Yuji,

On 07/03/2018 01:07, KUBOTA Yuji wrote:

Nice catch! I removed this unused variable.
http://cr.openjdk.java.net/~ykubota/8169358/webrev.04/

Best regards
Yuji



This looks good to me as well. I have imported your patch
and sent it to our test system and things are looking fine.

Could you prepare a final changeset with a properly formatted
comment [1], use jcheck [2] to verify that your changeset conforms
to the OpenJDK rules (no trailing whitespaces, no tabs etc...),
and upload it somewhere to http://cr.openjdk.java.net/~ykubota/8169358 ?

If there aren't any other review feedback in the mean time, I'll
be happy to sponsor your changes when that's ready.

best regards,

-- daniel
[1] http://openjdk.java.net/guide/producingChangeset.html
[2] http://openjdk.java.net/projects/code-tools/jcheck/

RE: httpserver does not close connections when RejectedExecutionException occurs

[Langer, Christoph]

Hi Yuji,

I had a look and to me it seems safe to do like webrev.03 suggests. That is, 
remove the throws clause. As Daniel pointed out, it seems that all callers of 
the "handle" method would do merely the same. The only thing I'm not sure about 
is the CancelledKeyException caught in line 413. In that case no exception 
would be logged currently. With your change, it would now inside "handle". 
Maybe you want to handle a CancelledKeyException explicitly in "handle" now and 
suppress the log?

What tests did you run?

BTW: you could remove line 396: ' boolean closed;' while you are touching this 
file. It is not needed.

Best regards
Christoph

> -Original Message-
> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
> KUBOTA Yuji
> Sent: Donnerstag, 1. März 2018 12:41
> To: OpenJDK Network Dev list <net-dev@openjdk.java.net>
> Cc: Yasumasa Suenaga <yasue...@gmail.com>
> Subject: Re: httpserver does not close connections when
> RejectedExecutionException occurs
> 
> Hi all,
> 
> Could you please review my patch(s)?
> 
> Thanks,
> Yuji
> 
> 2018-02-20 14:21 GMT+09:00 KUBOTA Yuji <kubota.y...@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.fu...@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
> >>

Re: httpserver does not close connections when RejectedExecutionException occurs

[KUBOTA Yuji]

Hi all,

Could you please review my patch(s)?

Thanks,
Yuji

2018-02-20 14:21 GMT+09:00 KUBOTA Yuji :
> 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 :
>> 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 :

 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 :
>
>
>> On 10 Nov 2016, at 14:43, Yasumasa Suenaga  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.
>

Re: httpserver does not close connections when RejectedExecutionException occurs

[KUBOTA Yuji]

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


> On 10 Nov 2016, at 14:43, Yasumasa Suenaga  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  wrote:
>>>
>>> Hi Yuji,
>>>
 http://cr.openjdk.java.net/~ykubota/8169358/webrev.01/
>>
>>

Re: httpserver does not close connections when RejectedExecutionException occurs

[Daniel Fuchs]

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 :

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 :



On 10 Nov 2016, at 14:43, Yasumasa Suenaga  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  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 :

* 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);
   

Re: httpserver does not close connections when RejectedExecutionException occurs

[KUBOTA Yuji]

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 :
> 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 :
>>
>>> On 10 Nov 2016, at 14:43, Yasumasa Suenaga  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  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 :
 * 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 = 

Re: httpserver does not close connections when RejectedExecutionException occurs

[KUBOTA Yuji]

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 :
>
>> On 10 Nov 2016, at 14:43, Yasumasa Suenaga  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  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 :
>>> * 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 

Re: httpserver does not close connections when RejectedExecutionException occurs

[Chris Hegarty]

> On 10 Nov 2016, at 14:43, Yasumasa Suenaga  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  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 :
>> * 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 

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

* 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

Re: httpserver does not close connections when RejectedExecutionException occurs

[Chris Hegarty]

> On 9 Nov 2016, at 12:38, Yasumasa Suenaga  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 :
 * 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 

Re: httpserver does not close connections when RejectedExecutionException occurs

[Yasumasa Suenaga]

Hi Yuji,


http://cr.openjdk.java.net/~ykubota/8169358/webrev.01/


I think you can use finally statement to close the connection in this case.
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.


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 :

* 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);

Re: httpserver does not close connections when RejectedExecutionException occurs

[KUBOTA Yuji]

Hi Chris,

If I can add reproduce code as test, could you please teach me a hint to add it?

Thanks,
Yuji.

2016-11-08 18:53 GMT+09:00 KUBOTA Yuji :
> 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 :
>>> * 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@37b50d9e rejected from
>>> java.util.concurrent.ThreadPoolExecutor@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()) {
>>> 

Re: httpserver does not close connections when RejectedExecutionException occurs

[Chris Hegarty]

Hi Yuji,

On 05/11/16 18:27, KUBOTA Yuji wrote:

Hi all,

com.sun.net.httpserver in jdk9 does not catch
RejectedExecutionException and it does not close connections. We must
catch this exception to close a socket.

Please review the following patch and reproduce steps.
If you agree with that this is an issue of jdk9, I create a new issue
on JBS. (I'm author)


Please file an issue for this.


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

-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@37b50d9e rejected from
java.util.concurrent.ThreadPoolExecutor@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