You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Rémy Maucherat <re...@apache.org> on 2018/10/15 14:49:24 UTC
Threads
Hi,
Looking at threads since there's a proliferation of executors and loose
threads, I notice a possible refactoring:
- start/stopExecutor is an obvious target, but by default is not there.
However, it does not delegate to the parent container if not there.
- The background thread is a loose thread, if the start/stopExecutor or
whatever it is was a ScheduledThreadPoolExecutor, then it can probably be
refactored.
- Some executors in the clustering code, they could be run by that
"start/stopExecutor" of thir parent container.
- As for the catalina.Executor(s) in the Service, it could still wrap all
this stuff, *if* our ThreadPoolExecutor used a ScheduledThreadPoolExecutor
instead (at least as an option).
So .. Options are:
a) Do nothing.
b) Merge everything except catalina.Executor into a new
ScheduledThreadPoolExecutor at the Container level (and delegate to
parent), so the Engine will have it by default. Given how the background
thread delay config and how startStopThreads work, this can remain mostly
compatible, except some internal API from the start/stop executor is going
to be gone.
c) Merge everything into one executor. Hopefully NIO2 users remember they
should give their connector its own executor.
So personally, I think b) is nice as the Excecutor is more for connector
pools configuration and we shouldn't pollute that with side uses. Is that
doable in 9, or should it be added to the "10" todo list ?
Comments ?
Rémy
Re: Threads
Posted by Rémy Maucherat <re...@apache.org>.
On Mon, Oct 15, 2018 at 5:26 PM Mark Thomas <ma...@apache.org> wrote:
> > and we shouldn't pollute that with side uses. Is that
> > doable in 9, or should it be added to the "10" todo list ?
>
> No strong view. If it can be done without impacting the external API
> then no objections here.
>
This looks ok compatibility wise. I will add this refactoring after 9.0.13.
Now, looking at things:
- There are two more heavyweight uses of an executor in clustering:
MessageDispatchInterceptor and RecieverBase. This might be moveable too
after review, IMO there's no gain to have multiple small internal pools.
- The async timeout threads of the connectors is an obvious item that could
be moved to that executor (it runs only once per second). I am adding the
necessary get/set APIs on ProtocolHandler and Endpoint.
Looking at other items:
- I could obviously avoid using a dedicated accept thread for NIO2 but it's
more complex. So maybe later.
- tribes.transport.bio ... Why does this still exist ?
Rémy
Re: Threads
Posted by Mark Thomas <ma...@apache.org>.
On 15/10/18 15:49, Rémy Maucherat wrote:
> Hi,
>
> Looking at threads since there's a proliferation of executors and loose
> threads, I notice a possible refactoring:
> - start/stopExecutor is an obvious target, but by default is not there.
> However, it does not delegate to the parent container if not there.
> - The background thread is a loose thread, if the start/stopExecutor or
> whatever it is was a ScheduledThreadPoolExecutor, then it can probably be
> refactored.
> - Some executors in the clustering code, they could be run by that
> "start/stopExecutor" of thir parent container.
> - As for the catalina.Executor(s) in the Service, it could still wrap all
> this stuff, *if* our ThreadPoolExecutor used a ScheduledThreadPoolExecutor
> instead (at least as an option).
>
> So .. Options are:
> a) Do nothing.
> b) Merge everything except catalina.Executor into a new
> ScheduledThreadPoolExecutor at the Container level (and delegate to
> parent), so the Engine will have it by default. Given how the background
> thread delay config and how startStopThreads work, this can remain mostly
> compatible, except some internal API from the start/stop executor is going
> to be gone.
> c) Merge everything into one executor. Hopefully NIO2 users remember they
> should give their connector its own executor.
>
> So personally, I think b) is nice as the Excecutor is more for connector
> pools configuration
+1
> and we shouldn't pollute that with side uses. Is that
> doable in 9, or should it be added to the "10" todo list ?
No strong view. If it can be done without impacting the external API
then no objections here.
Mark
>
> Comments ?
>
> Rémy
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Threads
Posted by Rémy Maucherat <re...@apache.org>.
On Wed, Nov 7, 2018 at 12:17 AM Rémy Maucherat <re...@apache.org> wrote:
> On Tue, Nov 6, 2018 at 6:39 PM Christopher Schultz <
> chris@christopherschultz.net> wrote:
>
>> > , and the javadoc says: If any execution of the task encounters an
>> > exception, subsequent executions are suppressed.
>> >
>> > So you think adding back the scheduling after a problem is better
>> > ?
>>
>> Yes, I think so. The container should protect itself from
>> applications, and this is a case where a misbehaving application can
>> break the correct operation of the container.
>>
>
> Ok, so it's harder, there's no API for it. I'll try to find a good
> solution.
>
Done for ContainerBase, it now uses another scheduled task to check the
state of the first one and looks clean IMO (a lot better as if there was
similar code attempting the same thing pre refactoring). It's probably
difficult to try to make the added code generic (it may need to check for
conditions, may use a different scheduling, etc) although I still might
attempt it at some point (although not as significant as this one, the
other scheduled tasks in Tomcat would be better off with a "raise dead"
capability).
Rémy
Re: Threads
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Nov 6, 2018 at 6:39 PM Christopher Schultz <
chris@christopherschultz.net> wrote:
> > Otherwise, at the moment, the behavior is the same as before: if
> > an exception remains uncaught like your OOM or ThreadDeath or
> > whatever, it will cancel the scheduling (and I haven't added the
> > code to add it back). I have chosen to use scheduleWithFixedDelay
> > (subtle tweaks can be made here: some of the calls could use
> > scheduleAtFixedRate instead, maybe like the connector async
> > timeout)
>
> I think scheduleWithFixedDelay makes more sense. If an application
> ties-up the background thread, you could have multiple background
> threads running on top of each other. Theoretically, everything those
> threads are doing should be thread-safe, but I think it's probably
> best to have only one running at a time.
>
scheduleAtFixedRate does not concurrently execute, it is possible it runs
late. Also, the container background threads are exclusive (they avoid
entering a container which has one enabled too). As a result, it should be
ok. I think scheduleAtFixedRate is only good for that async timeout though.
>
> > , and the javadoc says: If any execution of the task encounters an
> > exception, subsequent executions are suppressed.
> >
> > So you think adding back the scheduling after a problem is better
> > ?
>
> Yes, I think so. The container should protect itself from
> applications, and this is a case where a misbehaving application can
> break the correct operation of the container.
>
Ok, so it's harder, there's no API for it. I'll try to find a good solution.
Rémy
Re: Threads
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Rémy,
On 11/6/18 11:06 AM, Rémy Maucherat wrote:
> On Tue, Nov 6, 2018 at 4:18 PM Christopher Schultz <
> chris@christopherschultz.net> wrote:
>
>> This reply is quite late, I'm sorry for that.
>>
>> I'm not sure if this has changed since a while back, but it used
>> to be that an application could inadvertently kill the background
>> thread. One way to do that would be to trigger OOME or
>> StackOverflowError on the thread during some operation. An OOME
>> that wasn't actually fatal for the JVM because the objects were
>> all local to the thread and collectable once the exception was
>> thrown.
>>
>> If the background thread died, sessions would no longer expire,
>> which would eventually take everything down.
>>
>> If we are going to re-organize threads, I think it would be best
>> if there were protections against the above scenario...
>> basically, another thead watching the background threads. I think
>> you get that already with an ExecutorService and a periodic job,
>> so perhaps there isn't much to talk about.
>>
>
> The executor will deal with auto respawning up to its core threads,
> of course.
>
> Otherwise, at the moment, the behavior is the same as before: if
> an exception remains uncaught like your OOM or ThreadDeath or
> whatever, it will cancel the scheduling (and I haven't added the
> code to add it back). I have chosen to use scheduleWithFixedDelay
> (subtle tweaks can be made here: some of the calls could use
> scheduleAtFixedRate instead, maybe like the connector async
> timeout)
I think scheduleWithFixedDelay makes more sense. If an application
ties-up the background thread, you could have multiple background
threads running on top of each other. Theoretically, everything those
threads are doing should be thread-safe, but I think it's probably
best to have only one running at a time.
> , and the javadoc says: If any execution of the task encounters an
> exception, subsequent executions are suppressed.
>
> So you think adding back the scheduling after a problem is better
> ?
Yes, I think so. The container should protect itself from
applications, and this is a case where a misbehaving application can
break the correct operation of the container.
- -chris
>> I just wanted to keep that in mind.
>>
>> Thanks, - -chris -----BEGIN PGP SIGNATURE----- Comment: Using
>> GnuPG with Thunderbird - https://www.enigmail.net/
>>
>> iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlvhsLQACgkQHPApP6U8
>> pFhweRAAg3KnU+ThVhHMu+tKrXZwh7p7R4QmfokAIzLL08V1O1DW0fHtH+Z1KN0g
>> PsCjoKulluGUa8ByhSVFHKbACXP6aD5LpFFB9dyZZsGkI1SeRPEzm37ptIkzl7lV
>> /2wSydZ36XCb7+DoxUllgLu3lTT0FcoSXKWM3VcjjqeO2ChRUPJdygMpDUAfIcRo
>> yVfXpnfbWND0r6b9OG3UpFES6vw3NWHf7YP7IuWiB6tA+P4kBAZ2poQ0V/8stVJm
>> bbSu6t8sDH3jGGlMMmhpanSi9789IanyUi2hink/0MZwg25u8xqJly7Yz76nrwt6
>> +9shVGehBgZIHjEp9j77vePaigXXE9hK51j23Pk+98FEQ1PJiS5ZVZB8vag2d3Yq
>> C7ROAzMmZGMh3ymLcwWQgBV/FL1h5YNpHqGyL7IiqnSQKynkMm/2DmDYVgPoxymC
>> SwzLz2bPRHKMHIkqP20izj6lBo4KMHAFAqI7EFCeeCK7l/M2rGR0qeO/DrXwW0vd
>> nZt+2f+5WXxsH5oH+GP19EKB1blT9T0yD5AV02dBeDcjkhO/3ZITw4+Lp8J38LVi
>> 4mwlhSuodJVq1Ei6Ur3+LBBkPh6mBBnFChlJf0d2NmVwdNHhjfJuK4KnPIRvNdzk
>> oNgpEVCC0It9csaz8O8JTFpifiyMfIjqDQDypTKUDUXkEr6Sw60= =1fzA
>> -----END PGP SIGNATURE-----
>>
>> ---------------------------------------------------------------------
>>
>>
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>>
>
-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlvh0dwACgkQHPApP6U8
pFjbpQ//Z6BYDq32e9jkprGSyKYXJG1DaUUhYALvoxXuUYwRu0B2HGk7SIgPWLIw
hf8NOMzokRzwoTrnLRtrX1Z8C20KSVBPdIOCgXP0QcwRy0Zf/J29SWxeOq8aOrGp
URem/6bx8tQA60hDV4vF0atxHbkwco20O4Z6TPwvf2P9MygBMqjZX/z632wraL+/
nWeTWfVXL6bfnrGRDEUwJvmrl/9zrhYixA5ENXih/COi+K+WIyHrpoLgis9AzZVy
lEW7KNMOJdtERMMfbCgCQK4/BltCJoTTK1Ct0kk8aNqdH7inEME34qbrZFCumUr8
lXhkqMSxZF0QlJKBu15rUQI7oxGG9eZD6w2WpJ1jLvDv4onGGT1LJaUl+SGy8ZVC
NaNNvcYsHIZfPHD6jPMiFwEogc/Vs4bJIwqCCr9QaeQ4ZFmW1/skWMunfCBn83kR
pa2H0IvcJNk0lND0Y4fbE5qN4JQuBCMZ5d17a/bn1rEcUb9bUglo1ec3Q3Hl0wBz
s+IrFOdTXybC6HPgfP9Xk7w4Nwzf59ZLPMllxGUqOCaslLoTku3TI9SZvrIb7BlL
B1ixVhKOcBzoSRpp2ERFmf2eb3NxyIYmTj+E+WqpuzDbeE6TLKsegHy78UGk5ATS
y39SmVPAF3HaEX39G3xwDQZKavG/A8+utzV8SVVebQBmUmZ19DQ=
=jXKM
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Threads
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Nov 6, 2018 at 4:18 PM Christopher Schultz <
chris@christopherschultz.net> wrote:
> This reply is quite late, I'm sorry for that.
>
> I'm not sure if this has changed since a while back, but it used to be
> that an application could inadvertently kill the background thread.
> One way to do that would be to trigger OOME or StackOverflowError on
> the thread during some operation. An OOME that wasn't actually fatal
> for the JVM because the objects were all local to the thread and
> collectable once the exception was thrown.
>
> If the background thread died, sessions would no longer expire, which
> would eventually take everything down.
>
> If we are going to re-organize threads, I think it would be best if
> there were protections against the above scenario... basically,
> another thead watching the background threads. I think you get that
> already with an ExecutorService and a periodic job, so perhaps there
> isn't much to talk about.
>
The executor will deal with auto respawning up to its core threads, of
course.
Otherwise, at the moment, the behavior is the same as before: if an
exception remains uncaught like your OOM or ThreadDeath or whatever, it
will cancel the scheduling (and I haven't added the code to add it back). I
have chosen to use scheduleWithFixedDelay (subtle tweaks can be made here:
some of the calls could use scheduleAtFixedRate instead, maybe like the
connector async timeout), and the javadoc says: If any execution of the
task encounters an exception, subsequent executions are suppressed.
So you think adding back the scheduling after a problem is better ?
Rémy
>
> I just wanted to keep that in mind.
>
> Thanks,
> - -chris
> -----BEGIN PGP SIGNATURE-----
> Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
>
> iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlvhsLQACgkQHPApP6U8
> pFhweRAAg3KnU+ThVhHMu+tKrXZwh7p7R4QmfokAIzLL08V1O1DW0fHtH+Z1KN0g
> PsCjoKulluGUa8ByhSVFHKbACXP6aD5LpFFB9dyZZsGkI1SeRPEzm37ptIkzl7lV
> /2wSydZ36XCb7+DoxUllgLu3lTT0FcoSXKWM3VcjjqeO2ChRUPJdygMpDUAfIcRo
> yVfXpnfbWND0r6b9OG3UpFES6vw3NWHf7YP7IuWiB6tA+P4kBAZ2poQ0V/8stVJm
> bbSu6t8sDH3jGGlMMmhpanSi9789IanyUi2hink/0MZwg25u8xqJly7Yz76nrwt6
> +9shVGehBgZIHjEp9j77vePaigXXE9hK51j23Pk+98FEQ1PJiS5ZVZB8vag2d3Yq
> C7ROAzMmZGMh3ymLcwWQgBV/FL1h5YNpHqGyL7IiqnSQKynkMm/2DmDYVgPoxymC
> SwzLz2bPRHKMHIkqP20izj6lBo4KMHAFAqI7EFCeeCK7l/M2rGR0qeO/DrXwW0vd
> nZt+2f+5WXxsH5oH+GP19EKB1blT9T0yD5AV02dBeDcjkhO/3ZITw4+Lp8J38LVi
> 4mwlhSuodJVq1Ei6Ur3+LBBkPh6mBBnFChlJf0d2NmVwdNHhjfJuK4KnPIRvNdzk
> oNgpEVCC0It9csaz8O8JTFpifiyMfIjqDQDypTKUDUXkEr6Sw60=
> =1fzA
> -----END PGP SIGNATURE-----
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: Threads
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Rémy,
On 10/15/18 10:49, Rémy Maucherat wrote:
> Hi,
>
> Looking at threads since there's a proliferation of executors and
> loose threads, I notice a possible refactoring: -
> start/stopExecutor is an obvious target, but by default is not
> there. However, it does not delegate to the parent container if not
> there. - The background thread is a loose thread, if the
> start/stopExecutor or whatever it is was a
> ScheduledThreadPoolExecutor, then it can probably be refactored. -
> Some executors in the clustering code, they could be run by that
> "start/stopExecutor" of thir parent container. - As for the
> catalina.Executor(s) in the Service, it could still wrap all this
> stuff, *if* our ThreadPoolExecutor used a
> ScheduledThreadPoolExecutor instead (at least as an option).
>
> So .. Options are: a) Do nothing. b) Merge everything except
> catalina.Executor into a new ScheduledThreadPoolExecutor at the
> Container level (and delegate to parent), so the Engine will have
> it by default. Given how the background thread delay config and how
> startStopThreads work, this can remain mostly compatible, except
> some internal API from the start/stop executor is going to be
> gone. c) Merge everything into one executor. Hopefully NIO2 users
> remember they should give their connector its own executor.
>
> So personally, I think b) is nice as the Excecutor is more for
> connector pools configuration and we shouldn't pollute that with
> side uses. Is that doable in 9, or should it be added to the "10"
> todo list ?
>
> Comments ?
This reply is quite late, I'm sorry for that.
I'm not sure if this has changed since a while back, but it used to be
that an application could inadvertently kill the background thread.
One way to do that would be to trigger OOME or StackOverflowError on
the thread during some operation. An OOME that wasn't actually fatal
for the JVM because the objects were all local to the thread and
collectable once the exception was thrown.
If the background thread died, sessions would no longer expire, which
would eventually take everything down.
If we are going to re-organize threads, I think it would be best if
there were protections against the above scenario... basically,
another thead watching the background threads. I think you get that
already with an ExecutorService and a periodic job, so perhaps there
isn't much to talk about.
I just wanted to keep that in mind.
Thanks,
- -chris
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlvhsLQACgkQHPApP6U8
pFhweRAAg3KnU+ThVhHMu+tKrXZwh7p7R4QmfokAIzLL08V1O1DW0fHtH+Z1KN0g
PsCjoKulluGUa8ByhSVFHKbACXP6aD5LpFFB9dyZZsGkI1SeRPEzm37ptIkzl7lV
/2wSydZ36XCb7+DoxUllgLu3lTT0FcoSXKWM3VcjjqeO2ChRUPJdygMpDUAfIcRo
yVfXpnfbWND0r6b9OG3UpFES6vw3NWHf7YP7IuWiB6tA+P4kBAZ2poQ0V/8stVJm
bbSu6t8sDH3jGGlMMmhpanSi9789IanyUi2hink/0MZwg25u8xqJly7Yz76nrwt6
+9shVGehBgZIHjEp9j77vePaigXXE9hK51j23Pk+98FEQ1PJiS5ZVZB8vag2d3Yq
C7ROAzMmZGMh3ymLcwWQgBV/FL1h5YNpHqGyL7IiqnSQKynkMm/2DmDYVgPoxymC
SwzLz2bPRHKMHIkqP20izj6lBo4KMHAFAqI7EFCeeCK7l/M2rGR0qeO/DrXwW0vd
nZt+2f+5WXxsH5oH+GP19EKB1blT9T0yD5AV02dBeDcjkhO/3ZITw4+Lp8J38LVi
4mwlhSuodJVq1Ei6Ur3+LBBkPh6mBBnFChlJf0d2NmVwdNHhjfJuK4KnPIRvNdzk
oNgpEVCC0It9csaz8O8JTFpifiyMfIjqDQDypTKUDUXkEr6Sw60=
=1fzA
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org