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 2023/05/26 08:56:58 UTC
PersistentValve removal ?
Hi,
After looking at BZ 66513, reading things online, the javadoc with its
heavy handed usage warnings, code of PersistentValve and tracing back
all the calls in the PersistentManager itself, my conclusion is that
the functionality provided by that valve is probably not doable
without significant compromises that would likely break most of its
usefulness. This seems to be a core design issue basically, or at
least this is a far more complex problem than what can be achieved
using a quick hack.
As a result, I would be inclined to propose dropping PersistentValve
in Tomcat 11, and deprecating it in the other branches.
Comments ?
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: PersistentValve removal ?
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 5/30/23 12:46, Mark Thomas wrote:
> On 30/05/2023 15:10, Rémy Maucherat wrote:
>
>> Ok. I proposed the removal based on "It is intended to be used with
>> non-sticky load-balancers" from the javadoc. I think the clustering
>> and stickness have to be used instead of this valve, to comply with
>> all reasonable concurrency requirements from the container and
>> application in that use case. Basically the BZ can be fixed only if
>> there's one Tomcat. I understand this valve is in use since we got the
>> BZ, but given everything I don't understand how it is useful, I
>> considered users should either stop using it or move to session
>> clustering.
>
> Ah. Now I see why you were suggesting deprecation.
>
> Non-sticky LB implies more than one Tomcat instance.
>
> The requirement for no more than one concurrent request per session is
> something we can enforce on a single Tomcat node (with something like PR
> #623) but we can't enforce it across multiple nodes.
>
> Given we can't enforce it across multiple nodes, there is no point
> trying to enforce it on a single node.
+1
> Similarly, PersistentManager isn't going to work well with non-sticky
> load balancers. We should document that.
>
> The question becomes, is there a use case for the following:
>
> - multiple nodes
> - non-sticky LB
> - PersistentManager
> - client(s) guarantee of no more than one concurrent request per session
>
> The last point can't be enforced by Tomcat and stuff will break if a
> client breaks the guarantee.
I think that last point is one that maybe even the client cannot guarantee.
> If there is a use case, we should keep PersistentValve, document the
> requirements even more clearly that they are already.
>
> If there is no use case, we should remove PersistentValve from Tomcat 11
> and deprecate it in earlier versions as per your suggestion.
>
> Either way, BZ 66513 gets resolved as WONTFIX (more like can't fix).
Part of it we can sort-of fix... a single Tomcat can trip-over itself
which is not good. I started working on this in
https://github.com/apache/tomcat/pull/596 but I think I will close that
PR and instead propose subclasses of the DataSourceStore which use
different strategies for "replacing" session data in the database.
I think there is still scope for work on BZ 66513.
> I can't think of a use case that justifies keeping PersistentValve. BZ
> 66513 talks about non-sticky LB with containers. I'll ask some follow-up
> questions to see if there is something we are missing but my sense is
> that this was always broken but the users using it either never observed
> breakage or it happened rarely enough it wasn't followed up.
+1
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: PersistentValve removal ?
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 6/1/23 11:23, Mark Thomas wrote:
> On 30/05/2023 22:44, Rémy Maucherat wrote:
>> On Tue, May 30, 2023 at 6:46 PM Mark Thomas <ma...@apache.org> wrote:
>
> <snip/>
>
>>> I can't think of a use case that justifies keeping PersistentValve. BZ
>>> 66513 talks about non-sticky LB with containers. I'll ask some follow-up
>>> questions to see if there is something we are missing but my sense is
>>> that this was always broken but the users using it either never observed
>>> breakage or it happened rarely enough it wasn't followed up.
>>
>> +1
>> It sounds reasonable to redocument stating the limitations better,
>> deprecate, then remove in 11.
>
> The OP for BZ 66513 has indicated that the Valve is useful for them when
> running on containers in the cloud. I guess they must have a client that
> follows the no more than one concurrent request rule. Given that they
> explicitly asked us to keep the Valve, I'm happy to do so. I have a
> patch I need to apply that adds some debug logging they also asked for.
> I'll resolve the issue once that patch has been applied.
I will also re-work my patch for the Store such that it can avoid
duplicate-key violations in cases where users want "insert or update"
semantics (which will honestly probably be everybody). The problem is
that there is no one single way to do this that works across all RDBMS
systems.
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: PersistentValve removal ?
Posted by Mark Thomas <ma...@apache.org>.
On 30/05/2023 22:44, Rémy Maucherat wrote:
> On Tue, May 30, 2023 at 6:46 PM Mark Thomas <ma...@apache.org> wrote:
<snip/>
>> I can't think of a use case that justifies keeping PersistentValve. BZ
>> 66513 talks about non-sticky LB with containers. I'll ask some follow-up
>> questions to see if there is something we are missing but my sense is
>> that this was always broken but the users using it either never observed
>> breakage or it happened rarely enough it wasn't followed up.
>
> +1
> It sounds reasonable to redocument stating the limitations better,
> deprecate, then remove in 11.
The OP for BZ 66513 has indicated that the Valve is useful for them when
running on containers in the cloud. I guess they must have a client that
follows the no more than one concurrent request rule. Given that they
explicitly asked us to keep the Valve, I'm happy to do so. I have a
patch I need to apply that adds some debug logging they also asked for.
I'll resolve the issue once that patch has been applied.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: PersistentValve removal ?
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, May 30, 2023 at 6:46 PM Mark Thomas <ma...@apache.org> wrote:
>
> On 30/05/2023 15:10, Rémy Maucherat wrote:
>
> > Ok. I proposed the removal based on "It is intended to be used with
> > non-sticky load-balancers" from the javadoc. I think the clustering
> > and stickness have to be used instead of this valve, to comply with
> > all reasonable concurrency requirements from the container and
> > application in that use case. Basically the BZ can be fixed only if
> > there's one Tomcat. I understand this valve is in use since we got the
> > BZ, but given everything I don't understand how it is useful, I
> > considered users should either stop using it or move to session
> > clustering.
>
> Ah. Now I see why you were suggesting deprecation.
>
> Non-sticky LB implies more than one Tomcat instance.
>
> The requirement for no more than one concurrent request per session is
> something we can enforce on a single Tomcat node (with something like PR
> #623) but we can't enforce it across multiple nodes.
>
> Given we can't enforce it across multiple nodes, there is no point
> trying to enforce it on a single node.
>
> Similarly, PersistentManager isn't going to work well with non-sticky
> load balancers. We should document that.
>
> The question becomes, is there a use case for the following:
>
> - multiple nodes
> - non-sticky LB
> - PersistentManager
> - client(s) guarantee of no more than one concurrent request per session
>
> The last point can't be enforced by Tomcat and stuff will break if a
> client breaks the guarantee.
>
> If there is a use case, we should keep PersistentValve, document the
> requirements even more clearly that they are already.
>
> If there is no use case, we should remove PersistentValve from Tomcat 11
> and deprecate it in earlier versions as per your suggestion.
>
> Either way, BZ 66513 gets resolved as WONTFIX (more like can't fix).
Your fix is better than what I thought was possible, since I only
considered a map of lock objects (and good cleanup was a problem). I
think your patch should be merged.
> I can't think of a use case that justifies keeping PersistentValve. BZ
> 66513 talks about non-sticky LB with containers. I'll ask some follow-up
> questions to see if there is something we are missing but my sense is
> that this was always broken but the users using it either never observed
> breakage or it happened rarely enough it wasn't followed up.
+1
It sounds reasonable to redocument stating the limitations better,
deprecate, then remove in 11.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: PersistentValve removal ?
Posted by Mark Thomas <ma...@apache.org>.
On 30/05/2023 15:10, Rémy Maucherat wrote:
> Ok. I proposed the removal based on "It is intended to be used with
> non-sticky load-balancers" from the javadoc. I think the clustering
> and stickness have to be used instead of this valve, to comply with
> all reasonable concurrency requirements from the container and
> application in that use case. Basically the BZ can be fixed only if
> there's one Tomcat. I understand this valve is in use since we got the
> BZ, but given everything I don't understand how it is useful, I
> considered users should either stop using it or move to session
> clustering.
Ah. Now I see why you were suggesting deprecation.
Non-sticky LB implies more than one Tomcat instance.
The requirement for no more than one concurrent request per session is
something we can enforce on a single Tomcat node (with something like PR
#623) but we can't enforce it across multiple nodes.
Given we can't enforce it across multiple nodes, there is no point
trying to enforce it on a single node.
Similarly, PersistentManager isn't going to work well with non-sticky
load balancers. We should document that.
The question becomes, is there a use case for the following:
- multiple nodes
- non-sticky LB
- PersistentManager
- client(s) guarantee of no more than one concurrent request per session
The last point can't be enforced by Tomcat and stuff will break if a
client breaks the guarantee.
If there is a use case, we should keep PersistentValve, document the
requirements even more clearly that they are already.
If there is no use case, we should remove PersistentValve from Tomcat 11
and deprecate it in earlier versions as per your suggestion.
Either way, BZ 66513 gets resolved as WONTFIX (more like can't fix).
I can't think of a use case that justifies keeping PersistentValve. BZ
66513 talks about non-sticky LB with containers. I'll ask some follow-up
questions to see if there is something we are missing but my sense is
that this was always broken but the users using it either never observed
breakage or it happened rarely enough it wasn't followed up.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: PersistentValve removal ?
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, May 30, 2023 at 1:27 PM Mark Thomas <ma...@apache.org> wrote:
>
> On 29/05/2023 08:36, Mark Thomas wrote:
> > On 26/05/2023 09:56, Rémy Maucherat wrote:
> >> Hi,
> >>
> >> After looking at BZ 66513, reading things online, the javadoc with its
> >> heavy handed usage warnings, code of PersistentValve and tracing back
> >> all the calls in the PersistentManager itself, my conclusion is that
> >> the functionality provided by that valve is probably not doable
> >> without significant compromises that would likely break most of its
> >> usefulness. This seems to be a core design issue basically, or at
> >> least this is a far more complex problem than what can be achieved
> >> using a quick hack.
> >
> > I'd like to see if we can find an acceptable hack to enforce the no more
> > than one concurrent request per session rule. I have some ideas but need
> > to think them through to find all the edge cases I have missed.
> >
> >> As a result, I would be inclined to propose dropping PersistentValve
> >> in Tomcat 11, and deprecating it in the other branches.
> >
> > Given bug 66513 it seems that some folks do find it useful. If there is
> > a set of users for which the Valve is useful then I'd prefer to keep it
> > if we can (with clearly documented constraints).
> >
> > If t just isn't possible then removal / deprecation seems reasonable.
>
> How about this?
>
> https://github.com/apache/tomcat/pull/623
>
> I think this is an approach that could work. It addresses the
> thread-safety concerns with the first couple of attempts I had at
> solving this.
>
> If folks think that a) I haven't missed any other thread-safety issues
> and b) enforcing max one request per session is a viable approach then
> I'm happy to do the work to clean this up (docs, tests, etc.)
Ok. I proposed the removal based on "It is intended to be used with
non-sticky load-balancers" from the javadoc. I think the clustering
and stickness have to be used instead of this valve, to comply with
all reasonable concurrency requirements from the container and
application in that use case. Basically the BZ can be fixed only if
there's one Tomcat. I understand this valve is in use since we got the
BZ, but given everything I don't understand how it is useful, I
considered users should either stop using it or move to session
clustering.
Rémy
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: PersistentValve removal ?
Posted by Mark Thomas <ma...@apache.org>.
On 29/05/2023 08:36, Mark Thomas wrote:
> On 26/05/2023 09:56, Rémy Maucherat wrote:
>> Hi,
>>
>> After looking at BZ 66513, reading things online, the javadoc with its
>> heavy handed usage warnings, code of PersistentValve and tracing back
>> all the calls in the PersistentManager itself, my conclusion is that
>> the functionality provided by that valve is probably not doable
>> without significant compromises that would likely break most of its
>> usefulness. This seems to be a core design issue basically, or at
>> least this is a far more complex problem than what can be achieved
>> using a quick hack.
>
> I'd like to see if we can find an acceptable hack to enforce the no more
> than one concurrent request per session rule. I have some ideas but need
> to think them through to find all the edge cases I have missed.
>
>> As a result, I would be inclined to propose dropping PersistentValve
>> in Tomcat 11, and deprecating it in the other branches.
>
> Given bug 66513 it seems that some folks do find it useful. If there is
> a set of users for which the Valve is useful then I'd prefer to keep it
> if we can (with clearly documented constraints).
>
> If t just isn't possible then removal / deprecation seems reasonable.
How about this?
https://github.com/apache/tomcat/pull/623
I think this is an approach that could work. It addresses the
thread-safety concerns with the first couple of attempts I had at
solving this.
If folks think that a) I haven't missed any other thread-safety issues
and b) enforcing max one request per session is a viable approach then
I'm happy to do the work to clean this up (docs, tests, etc.)
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: PersistentValve removal ?
Posted by Mark Thomas <ma...@apache.org>.
On 26/05/2023 09:56, Rémy Maucherat wrote:
> Hi,
>
> After looking at BZ 66513, reading things online, the javadoc with its
> heavy handed usage warnings, code of PersistentValve and tracing back
> all the calls in the PersistentManager itself, my conclusion is that
> the functionality provided by that valve is probably not doable
> without significant compromises that would likely break most of its
> usefulness. This seems to be a core design issue basically, or at
> least this is a far more complex problem than what can be achieved
> using a quick hack.
I'd like to see if we can find an acceptable hack to enforce the no more
than one concurrent request per session rule. I have some ideas but need
to think them through to find all the edge cases I have missed.
> As a result, I would be inclined to propose dropping PersistentValve
> in Tomcat 11, and deprecating it in the other branches.
Given bug 66513 it seems that some folks do find it useful. If there is
a set of users for which the Valve is useful then I'd prefer to keep it
if we can (with clearly documented constraints).
If t just isn't possible then removal / deprecation seems reasonable.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org