[weld-dev] Propagation of org.jboss.weld.context.ConversationContext.conversations through session failover

Matej Novotny manovotn at redhat.com
Wed May 13 10:38:19 EDT 2020 

That's a very good catch! You're right.

Tracking this back, I think I understand why and how it works the way it does now.

That JBoss issue was a known problem before a rework of session replication - after that it was no longer an issue and that, I suppose,
was the reason why we still had WELD-1130 open and unsolved. I guess we didn't hear any complaints from other EE servers until you came
and implemented WELD-1130 using the added config option 'RESET_HTTP_SESSION_ATTR_ON_BEAN_ACCESS'.

In the light of that I think the correct way to solve this would be a PR just like Benjamin sent initially - https://github.com/weld/core/pull/1983/files
Implement aggressive re-storing of conversation map in the session but gate it behind the aforementioned configuration property (so that default Weld behaviour stays intact).
I've already created a WELD issue for it - https://issues.redhat.com/browse/WELD-2626

Feel free to send PRs.

Regards
Matej



----- Original Message -----
> From: "Allan Zhang" <zhang at ca.ibm.com>
> To: "Matej Novotny" <manovotn at redhat.com>
> Cc: "Benjamin Confino" <BENJAMIC at uk.ibm.com>, "Shinji Ohtsuka" <EB92769 at jp.ibm.com>, "Emily Jiang"
> <EMIJIANG at uk.ibm.com>, weld-dev at lists.jboss.org
> Sent: Tuesday, May 12, 2020 6:26:34 PM
> Subject: RE: [weld-dev] Propagation of org.jboss.weld.context.ConversationContext.conversations through session
> failover
> 
> 
> I thought this was an old bug that resolved already
> https://issues.redhat.com/browse/WELD-1130
> 
> Similarly It was documented under know issue of JBPAPP6-1326 - CDI beans
> with SET replication trigger are not replicating
> https://access.redhat.com/documentation/en-us/jboss_enterprise_application_platform/6/html/release_notes_6.0.1/ar01s06
> @Benjamin Can the customer use SET_AND_NON_PRIMITIVE_GET setting?
> 
> Thanks,
> Allan Zhang
> 
> 
> 
> From:	Matej Novotny <manovotn at redhat.com>
> To:	Benjamin Confino <BENJAMIC at uk.ibm.com>
> Cc:	Shinji Ohtsuka <EB92769 at jp.ibm.com>, Emily Jiang
>             <EMIJIANG at uk.ibm.com>, weld-dev at lists.jboss.org, Allan Zhang
>             <zhang at ca.ibm.com>
> Date:	2020-05-12 09:10 AM
> Subject:	[EXTERNAL] Re: [weld-dev] Propagation of
>             org.jboss.weld.context.ConversationContext.conversations
>             through session failover
> 
> 
> 
> @Allan
> I read part 7.7.2 of the spec and I see no indications of such requirement
> there.
> FYI, you are probably not subscribed to weld-dev list and your emails are
> not getting through (but I am getting them as I am in the CC) -
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.jboss.org_pipermail_weld-2Ddev_2020-2DMay_date.html&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=xixRx94dLWgNDRQdrEdGkA&m=SUkgCDS7nlw4OjHkMdXC9Ui72gAJLr_Yhxe8q1QMeQo&s=xROr9haHV8Og2JsxNmC_8vtFTEtiLkNoGD75SupiMlc&e=
> 
> 
> @Benjamin
> > I'm investigating what can be done on our end, but since it is a
> > grey area shouldn't weld err on the side of following Oracle's advice? Or
> > at least include a setting to switch to following the advice?
> 
> As for Weld, the conversation map is not the only thing we store in
> session.
> For instance, whole session context is stored there (every single bean).
> Even conversation context might be stored there and maybe something else
> but from the top of my head, these are quite prominent.
> And we also do not call setAttribute() every single time a bean changes or
> is accessed. That would be massive perf overhead.
> What I mean is that the issue you are seeing is a tip of an iceberg - it
> looks easy to fix for this one scenario/case, but it wouldn't be a complete
> change as multiple other things function the same way and it would require
> bigger code changes that I don't think are worth it.
> What you are asking for is, simply said, an unsupported replication mode.
> 
> Unless you can change the way Liberty treats mutable attributes in session,
> your customer should probably swap to a mode where all attributes are
> stored/replicated on shutdown.
> 
> Matej
> 
> ----- Original Message -----
> > From: "Benjamin Confino" <BENJAMIC at uk.ibm.com>
> > To: "Matej Novotny" <manovotn at redhat.com>
> > Cc: "Shinji Ohtsuka" <EB92769 at jp.ibm.com>, "Emily Jiang"
> <EMIJIANG at uk.ibm.com>, weld-dev at lists.jboss.org, "Allan
> > Zhang" <zhang at ca.ibm.com>
> > Sent: Monday, May 11, 2020 11:02:04 PM
> > Subject: RE: [weld-dev] Propagation of
> org.jboss.weld.context.ConversationContext.conversations through session
> > failover
> >
> > Hello
> >
> > I spoke to Allan and he agrees with you that it is a grey area in the
> > spec. I'm investigating what can be done on our end, but since it is a
> > grey area shouldn't weld err on the side of following Oracle's advice? Or
> > at least include a setting to switch to following the advice? Especially
> > since it would be technically simple to do so; as the map is only
> modified
> > in one class and Allan says it would be a good performance improvement.
> >
> > Regards
> > Benjamin
> >
> >
> >
> > From:   Matej Novotny <manovotn at redhat.com>
> > To:     Benjamin Confino <BENJAMIC at uk.ibm.com>
> > Cc:     Allan Zhang <zhang at ca.ibm.com>, Shinji Ohtsuka
> > <EB92769 at jp.ibm.com>, Emily Jiang <EMIJIANG at uk.ibm.com>,
> > weld-dev at lists.jboss.org
> > Date:   11/05/2020 09:33
> > Subject:        [EXTERNAL] Re: [weld-dev] Propagation of
> > org.jboss.weld.context.ConversationContext.conversations through session
> > failover
> >
> >
> >
> > The document you linked is not a specification itself and as such is not
> > binding. Do you have an actual specification bits that prove this?
> >
> > I went looking into servlet specification[1] and asked someone from WFLY
> > who has more expertise in this field to also take a look.
> > None of us found anything stating that you are required to call
> > setAttribute() every time you change mutable attribute.
> > In fact there is very little about attributes (or "replication triggers"
> > for attributes) and you could say this is grey area.
> >
> > To me it still makes way more sense to distinguish between
> > mutable/immutable attributes and for mutable ones you should set "dirty"
> > state even on read, not just write.
> >
> > Matej
> >
> _____________________________________________________________________________________
> 
> > [1]
> >
> https://urldefense.proofpoint.com/v2/url?u=https-3A__javaee.github.io_servlet-2Dspec_downloads_servlet-2D4.0_servlet-2D4-5F0-5FFINAL.pdf&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=hl8XiFr1UrTSgyXVc4KO_i51sM3Gyhlu1KZ4TkyseEQ&m=CfpQw81lIS0WnlUkCYtynvtzj_Ee225qD2GwdUvOIxI&s=rZbvOFfcelMTj2PXVwldp1g7nIT98q44qV1HMDyVXRc&e=
> 
> >
> >
> > ----- Original Message -----
> > > From: "Benjamin Confino" <BENJAMIC at uk.ibm.com>
> > > To: "Allan Zhang" <zhang at ca.ibm.com>
> > > Cc: "Matej Novotny" <manovotn at redhat.com>, "Shinji Ohtsuka"
> > <EB92769 at jp.ibm.com>, "Emily Jiang"
> > > <EMIJIANG at uk.ibm.com>, weld-dev at lists.jboss.org
> > > Sent: Thursday, May 7, 2020 12:41:01 PM
> > > Subject: RE: [weld-dev] Propagation of
> > org.jboss.weld.context.ConversationContext.conversations through session
> > > failover
> > >
> > > Hello
> > >
> > > Based on Allan Zhang's comments I have closed my pull requests and
> > opened
> > > this one
> >
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_weld_core_pull_1990&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=hl8XiFr1UrTSgyXVc4KO_i51sM3Gyhlu1KZ4TkyseEQ&m=CfpQw81lIS0WnlUkCYtynvtzj_Ee225qD2GwdUvOIxI&s=1umNSumSqXXSSvqtWTV7vcN5vNDvSkfrcpDcSuRPvw8&e=
> 
> >  which is a better match
> > > for the recommendations in the sessionmanagement spec. I still have not
> > > heard back from my customer so I will compile an updated test fix and
> > > chase them up. If they approve, or if they do not reply soon I will
> > create
> > > pull requests for the other branches.
> > >
> > > Regards
> > > Benjamin
> > >
> > >
> > >
> > > From:   Allan Zhang/Toronto/IBM
> > > To:     Matej Novotny <manovotn at redhat.com>
> > > Cc:     Benjamin Confino <BENJAMIC at uk.ibm.com>, Shinji Ohtsuka
> > > <EB92769 at jp.ibm.com>, Emily Jiang <EMIJIANG at uk.ibm.com>,
> > > weld-dev at lists.jboss.org
> > > Date:   06/05/2020 17:42
> > > Subject:        Re: [EXTERNAL] Re: [weld-dev] Propagation of
> > > org.jboss.weld.context.ConversationContext.conversations through
> session
> > > failover
> > >
> > >
> > >
> > > >Shouldn't Liberty consider (at least after restart) any access to a
> > > collection-based session property as an action marking that property
> > > "dirty"?
> > >
> > > Liberty already has write option of ALL_SESSION_ATTRIBUTES to mark the
> > > property "dirty" but customer wont use it for performance reasons.
> > >
> > > The PR fix is following the JEE spec where property "dirty" require
> > > setAttribute() call.
> > >
> >
> https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.oracle.com_cd_E13924-5F01_coh.340_e13819_sessionmanagement.htm&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=hl8XiFr1UrTSgyXVc4KO_i51sM3Gyhlu1KZ4TkyseEQ&m=CfpQw81lIS0WnlUkCYtynvtzj_Ee225qD2GwdUvOIxI&s=ZWrBJpUbA6i2VK0orPqJejDuIgd6tbSKtEGnDJSzopY&e=
> 
> >
> > >
> > > As a general rule, all session attributes should be treated as
> immutable
> > > objects if possible. This ensures that developers are consciously aware
> > > when they change attributes. With mutable objects, modifying attributes
> > > often requires two steps: modifying the state of the attribute object,
> > and
> > > then manually updating the session with the modified attribute object
> by
> > > calling javax.servlet.http.HttpSession.setAttribute(). This means that
> > > your application should always call setAttribute() if the attribute
> > value
> > > has been changed, otherwise, the modified attribute value will not
> > > replicate to the backup server.
> > >
> > > Thanks
> > > Allan Zhang
> > >
> > >
> > >
> > >
> > > From:   Matej Novotny <manovotn at redhat.com>
> > > To:     Benjamin Confino <BENJAMIC at uk.ibm.com>
> > > Cc:     Shinji Ohtsuka <EB92769 at jp.ibm.com>, Emily Jiang
> > > <EMIJIANG at uk.ibm.com>, weld-dev at lists.jboss.org, Allan Zhang
> > > <zhang at ca.ibm.com>
> > > Date:   2020-05-06 07:52 AM
> > > Subject:        [EXTERNAL] Re: [weld-dev] Propagation of
> > > org.jboss.weld.context.ConversationContext.conversations through
> session
> > > failover
> > >
> > >
> > >
> > > Hi,
> > >
> > > thanks for explanation, I understand the flow now.
> > >
> > > However, your suggested PR still looks like a workaround more than a
> fix
> > > to me.
> > >
> > > I doubt Weld is the only case where this will emerge. You can easily
> > store
> > > any collection in session and expect it to behave like we do.
> > > So in the very least, anything of type java.util.Collection that is
> > stored
> > > in session is susceptible to this behaviour.
> > > Shouldn't Liberty consider (at least after restart) any access to a
> > > collection-based session property as an action marking that property
> > > "dirty"?
> > >
> > > Regards
> > > Matej
> > >
> > > ----- Original Message -----
> > > > From: "Benjamin Confino" <BENJAMIC at uk.ibm.com>
> > > > To: "Matej Novotny" <manovotn at redhat.com>
> > > > Cc: "Shinji Ohtsuka" <EB92769 at jp.ibm.com>, "Emily Jiang"
> > > <EMIJIANG at uk.ibm.com>, weld-dev at lists.jboss.org, "Allan
> > > > Zhang" <zhang at ca.ibm.com>
> > > > Sent: Tuesday, May 5, 2020 3:59:30 PM
> > > > Subject: RE: [weld-dev] Propagation of
> > > org.jboss.weld.context.ConversationContext.conversations through
> session
> > > > failover
> > > >
> > > > Hello Matej
> > > >
> > > > No worries about the delay, my customer has not responded so you are
> > > ahead
> > > > of schedule. You are correct, the purpose of this PR is to trigger
> the
> > > > "dirty" state in liberty's session storage. Let me try and describe
> > the
> > > > flow again and hopefully it will be clearer:
> > > >
> > > >
> > > > A1) The application is started for the first time. A conversation map
> > is
> > > > created and stored into the session context. This marks it as dirty.
> > > > A2) The application puts stuff into the map. Lets say this leaves the
> > > map
> > > > in State A
> > > > A3) The server is told to shut down.
> > > > A4) Liberty sees the map is dirty and saves map State A to persistent
> > > > storage.
> > > >
> > > > B1) The server starts again.
> > > > B2) Weld retrieves the conversation map (State A) from persistent
> > > storage.
> > > > Since it already has a map it does not put anything into session
> > > storage.
> > > > Thus the map is clean.
> > > > B3) The application does more stuff, moving the map to (State B).
> > > > B3) The server is told to shut down.
> > > > B4) Liberty sees the map is clean, it doesn't transfer anything to
> > > > persistent storage.
> > > >
> > > > C1) The server starts again.
> > > > C2) Weld retrieves the conversation map (State A). This is the wrong
> > > > state.
> > > >
> > > >
> > > > My PR changes B2 so that Weld will set the same map back into session
> > > > storage. Then in B4 liberty will see the map is dirty and save State
> B
> > > to
> > > > storage, ensuring the correct state is retrieved at C2.
> > > >
> > > > We do have alternative solutions, for example you can configure
> > Liberty
> > > to
> > > > store everything regardless of whether it is dirty or not. However
> > this
> > > > has performance implications and so my customer does not want to use
> > > that
> > > > option.
> > > >
> > > > Regards
> > > > Benjamin
> > > >
> > > >
> > > >
> > > >
> > > > From:   Matej Novotny <manovotn at redhat.com>
> > > > To:     Benjamin Confino <BENJAMIC at uk.ibm.com>
> > > > Cc:     weld-dev at lists.jboss.org, Allan Zhang <zhang at ca.ibm.com>,
> > Shinji
> > > > Ohtsuka <EB92769 at jp.ibm.com>, Emily Jiang <EMIJIANG at uk.ibm.com>
> > > > Date:   05/05/2020 12:17
> > > > Subject:        [EXTERNAL] Re: [weld-dev] Propagation of
> > > > org.jboss.weld.context.ConversationContext.conversations through
> > session
> > > > failover
> > > >
> > > >
> > > >
> > > > I finally found some time to look into this, it took longer than I
> > > > anticipated, sorry for that.
> > > >
> > > > Firstly, I am not quite following the flow of the reproducer you
> > > > described.
> > > > Weld creates the conversation map and stores in into the session on
> > > > creation. We then retrieve this map from session whenever needed and
> > > store
> > > > additional things into it.
> > > > All the while we use the same map (the same reference, or object if
> > you
> > > > will)....so Liberty should see the same state in it, right? Or am I
> > > > missing something obvious?
> > > >
> > > > I am not familiar with how Liberty deals with server shutdown and how
> > it
> > > > stores session attributes, but i would expect that if you just grab
> > the
> > > > existing map from there, it will be up to date.
> > > > The PRs you sent seem superfluous to me - you are overriding the
> > > attribute
> > > > with exactly the same thing. probably just to trigger "dirty" state
> in
> > > > your session storage?
> > > >
> > > > Regards
> > > > Matej
> > > >
> > > > ----- Original Message -----
> > > > > From: "Benjamin Confino" <BENJAMIC at uk.ibm.com>
> > > > > To: "Matej Novotny" <manovotn at redhat.com>
> > > > > Cc: weld-dev at lists.jboss.org, "Allan Zhang" <zhang at ca.ibm.com>,
> > > "Shinji
> > > > Ohtsuka" <EB92769 at jp.ibm.com>, "Emily Jiang"
> > > > > <EMIJIANG at uk.ibm.com>
> > > > > Sent: Wednesday, April 29, 2020 11:56:43 AM
> > > > > Subject: RE: [weld-dev] Propagation of
> > > > org.jboss.weld.context.ConversationContext.conversations through
> > session
> > > > > failover
> > > > >
> > > > > Thank you for the heads up. When it's time to think about delivery
> > > I'll
> > > > be
> > > > > sure to create a PR against 3.1 and I presume you'd like me to
> leave
> > > > > master alone for now?
> > > > >
> > > > > Regards
> > > > > Benjamin
> > > > >
> > > > >
> > > > >
> > > > > From:   Matej Novotny <manovotn at redhat.com>
> > > > > To:     Benjamin Confino <BENJAMIC at uk.ibm.com>
> > > > > Cc:     weld-dev at lists.jboss.org, Allan Zhang <zhang at ca.ibm.com>,
> > > Shinji
> > > > > Ohtsuka <EB92769 at jp.ibm.com>, Emily Jiang <EMIJIANG at uk.ibm.com>
> > > > > Date:   29/04/2020 09:51
> > > > > Subject:        [EXTERNAL] Re: [weld-dev] Propagation of
> > > > > org.jboss.weld.context.ConversationContext.conversations through
> > > session
> > > > > failover
> > > > >
> > > > >
> > > > >
> > > > > Hi,
> > > > >
> > > > > I'll take a look later today.
> > > > > Note that master branch is no longer Weld 3.x, it is 4.x (Jakarta
> EE
> > > 9)
> > > > > and the CI there is going bonkers yet as I am in the middle of
> > > changing
> > > > > it.
> > > > > If you want to file a PR against Weld 3, you can use 3.1 branch for
> > > > that.
> > > > >
> > > > > Regards
> > > > > Matej
> > > > >
> > > > > ----- Original Message -----
> > > > > > From: "Benjamin Confino" <BENJAMIC at uk.ibm.com>
> > > > > > To: weld-dev at lists.jboss.org
> > > > > > Cc: "Allan Zhang" <zhang at ca.ibm.com>, "Shinji Ohtsuka"
> > > > > <EB92769 at jp.ibm.com>, "Emily Jiang" <EMIJIANG at uk.ibm.com>
> > > > > > Sent: Tuesday, April 28, 2020 2:31:44 PM
> > > > > > Subject: [weld-dev] Propagation of
> > > > > org.jboss.weld.context.ConversationContext.conversations through
> > > session
> > > > > failover
> > > > > >
> > > > > > Hello weld
> > > > > >
> > > > > > I had a customer report that they were getting conversation not
> > > found
> > > > > > exceptions when restarting their server and visiting a url with a
> > > > ?cid=1
> > > > > > suffix.
> > > > > >
> > > > > > After investigation I believe the issue is that weld was
> acquiring
> > > > it's
> > > > > > ConversationContext.conversations from the session database via
> > > > > > com.ibm.ws.session.store.db.DatabaseSession.getMultiRowAppData().
> > > Once
> > > > > weld
> > > > > > had retrieved the conversations map it would then decide that
> > since
> > > > the
> > > > > map
> > > > > > was already in the session attributes there was no need to put it
> > > back
> > > > > into
> > > > > > the attributes.
> > > > > >
> > > > > > This means that Liberty did not realise the conversations map had
> > > been
> > > > > > updated, and did not store it's updated state into the database
> > when
> > > > the
> > > > > > server shut down again.
> > > > > >
> > > > > > I have submitted a pair of pull requests that asks weld to mark
> > the
> > > > > > conversation map as dirty upon access - this behaviour is gated
> > > behind
> > > > > > ConfigurationKey.RESET_HTTP_SESSION_ATTR_ON_BEAN_ACCESS - I have
> > > > tested
> > > > > it
> > > > > > locally and it works. The next step is to prepare a test fix for
> > the
> > > > > > customer to verify. However I wanted to send you this quick note
> > to
> > > > keep
> > > > > you
> > > > > > in the loop.
> > > > > >
> > > > > > Regards
> > > > > > Benjamin
> > > > > > Unless stated otherwise above:
> > > > > > IBM United Kingdom Limited - Registered in England and Wales with
> > > > number
> > > > > > 741598.
> > > > > > Registered office: PO Box 41, North Harbour, Portsmouth,
> Hampshire
> > > PO6
> > > > > 3AU
> > > > > >
> > > > > > _______________________________________________
> > > > > > weld-dev mailing list
> > > > > > weld-dev at lists.jboss.org
> > > > > >
> > > > >
> > > >
> > >
> >
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.jboss.org_mailman_listinfo_weld-2Ddev&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=hl8XiFr1UrTSgyXVc4KO_i51sM3Gyhlu1KZ4TkyseEQ&m=WVR6Rz8X_vITi7c26XiQc9JlPkPasM6Px9gWE5r-pTg&s=qyzWuJ4v2pIKB7CZ39-u649jw6ouFAc8zecTq1XY1-k&e=
> 
> >
> > >
> > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > Unless stated otherwise above:
> > > > > IBM United Kingdom Limited - Registered in England and Wales with
> > > number
> > > > > 741598.
> > > > > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
> > PO6
> > > > 3AU
> > > > >
> > > >
> > > >
> > > >
> > > >
> > > > Unless stated otherwise above:
> > > > IBM United Kingdom Limited - Registered in England and Wales with
> > number
> > > > 741598.
> > > > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
> PO6
> > > 3AU
> > > >
> > >
> > >
> > >
> > >
> > >
> > > Unless stated otherwise above:
> > > IBM United Kingdom Limited - Registered in England and Wales with
> number
> > > 741598.
> > > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6
> > 3AU
> > >
> >
> >
> >
> >
> > Unless stated otherwise above:
> > IBM United Kingdom Limited - Registered in England and Wales with number
> > 741598.
> > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6
> 3AU
> >
> 
> 
> 
>

More information about the weld-dev mailing list