Re: PersistentManager, PersistentValve, and DataSource/JDBCStore can cause PK violations

[Christopher Schultz]

Romain,

On 3/8/23 04:10, Romain Manni-Bucau wrote:
Seems doing it only there will get back to the issue the synchronization
was introduced (there are other synchronized(session) for "local" instance).

Don't forget that there are multiple reasons to synchronize on a 
session, and they solve different problems. Some examples:

1. Don't corrupt the internal state of the Session

synchronized(session) {
  Integer count = (Integer)session.getAttribute("count");
  count = count +1;
  session.setAttribute("count", count);
}

This ensures that you don't "count twice" or fail to count even once 
when there are multiple threads in play. There are application business 
reasons to do this kind of thing, and this doesn't solve them all (e.g. 
across a cluster, or for different threads working on the "same" session 
but with different JVM objects representing them).

If there are multiple JVM objects in play, this is still appropriate 
because it solves the immediate problem.

2. Don't do something with the session simultaneously that's Important

That's where this proposal falls: the PresistentManager knows it only 
wants to do something very specific, here, and no other code needs to 
know about it or cares about it.

So I think it's okay not to worry about /other/ instances of 
synchronized(session) in the code and having to update those. Those are 
being done for a different reason.

However you hit a real point, the instance does not have to be stable, only
its equals/hashCode could be considered stable so guess the idea would be
to add to the session *manager* a getLock() method (don't think a RWLock is
needed there due to current usage) and use it instead of synchronized.

The synchronization mechanism itself isn't super important, it's just 
that we want to:

1. Perform operations for this one session (regardless of how many JVM 
objects are present in memory)

2. Allow other sessions to continue without interfering with them

It would be easy to just synchronzied(this) and never have a problem, 
but that kills performance.

The lock eviction should get some kind of delay for its own eviction and
not be evicted directly with the session to work.
A trivial impl could be to use a Map<$sessionId, $lock> and add a default
delay of 0 when purely in memory and a few seconds when a remote manager is
used (this logic belonging to the manager).

Are you talking about a distributed Manager that can handle 
cross-cluster synchronization?

A Map of sessionId -> Lock would work locally, and may be the better 
solution but it does require more memory, more management, 
synchronization (or ConcurrentMap, etc.), etc.

That said it stays a workaround and a better protocol handling that in a
cluster can be a more robust (but more complex) solution so not sure which
compromise is the best.

I'd like to solve the single-JVM case first, here, but I think you have 
some interesting ideas for the future.

If I were implementing a clustered application, I would not choose 
HttpSession as the way to share data across the cluster, for exactly 
these reasons.

-chris

Le mer. 8 mars 2023 à 04:43, Han Li <li...@apache.org> a écrit :



On Mar 8, 2023, at 07:29, Christopher Schultz <
ch...@christopherschultz.net> wrote:

All,

Please see https://bz.apache.org/bugzilla/show_bug.cgi?id=66513 for
reference.

It appears that the synchronization used by the PersistentManager can
cause problems when used with the PersistentValve and DataSource/JDBCStore.

The problem is that PersistentManager assumes that the Session object
can be used as a synchronization monitor to load/update the session in the
Store. The DataSource/JDBCStore implementation uses an INSERT to create a
new session, and a DELETE-then-INSERT to re-write the session data in the
db.

When two requests arrive simultaneously, thread scheduling can cause
DELETE-then-DELETE-then-INSERT-then-INSERT which causes a duplicate PK
violation.

If the PersistentValve were not in use, the in-memory Session would be
stable. PersistentValve re-loads the Session from the Store on ever
request, rendering the PersistentManager's synchronized(session) attempt to
protect things useless.

I think a simple way to fix this might be to change:

// PersistentManager.java:478~
        if(session != null) {
            synchronized(session){
                session = super.findSession(session.getIdInternal());
                if(session != null){
                   // To keep any external calling code from messing up
the
                   // concurrency.
                   session.access();
                   session.endAccess();
                }
            }
        }

to this:

        if(session != null) {
            sessionId = String.intern(sessionId);
            synchronized(sessionId){
                session = super.findSession(session.getIdInternal());
                if(session != null){
                   // To keep any external calling code from messing up
the
                   // concurrency.
                   session.access();
                   session.endAccess();
                }
            }
        }

This swaps the Session object for the sessionId as the synchronization
monitor. We use String.intern to ensure that we always have the same exact
object, even across sessions, request, etc.
-1

This method does seem very simple and solves this problem, but it’s not as
good as you might think, see below:
https://shipilev.net/jvm/anatomy-quarks/10-string-intern/ <
https://shipilev.net/jvm/anatomy-quarks/10-string-intern/>

So I don’t think it should be the preferred option.

Han


This is *a* way to solve this problem. There are other ways.

Another way is also a TODO in the DataSourceRealm code which suggests
using UPDATE for sessions that already exist. That is probably worth
implementing, and it would fix this particular issue.

Note that it is essentially impossible to prevent thread scheduling,
requests to other members of the cluster, etc. to prevent data-loss from
the session and this BZ isn't asking us to fix that. It's only asking that
a single Tomcat node with PersistentValve enabled doesn't cause thee
duplicate PK violations for some pretty basic usages.

-chris

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org





---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org