Le mer. 8 mars 2023 à 16:23, Christopher Schultz <
ch...@christopherschultz.net> a écrit :
> 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.
>
Hmm, i didn't check recently but this was not guaranteed by servlet spec,
is it - if so the issue you describe wouldn't be probably too?
>
> 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?
>
No, the (concurrent)map solution was really for local instances just adding
to support a small latency in session destruction/recreation.
>
> 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.
>
For me it is not a big deal when you use session because it is exactly the
issue you hit with session themselves (in a worse manner since sessions are
not "a few session" latent objects) so sounds/sounded like a compromise
short term.
>
> > 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.
>
+1
>
> -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
>
>
