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