Hi Chris, Seems doing it only there will get back to the issue the synchronization was introduced (there are other synchronized(session) for "local" instance). 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 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).
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. Romain Manni-Bucau @rmannibucau <https://twitter.com/rmannibucau> | Blog <https://rmannibucau.metawerx.net/> | Old Blog <http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> | LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book <https://www.packtpub.com/application-development/java-ee-8-high-performance> 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 > > > >
