I created https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6751
Feedback would be very welcome. Best regards, Thomas On Thu, Feb 27, 2020 at 10:08 AM Thomas Heigl <tho...@umschalt.com> wrote: > Hi all, > > I can confirm that my analysis was correct. I implemented a quick version > of ApplicationScopedPageAccessSynchronizer and everything works as expected. > > Providing a custom PageAccessSynchronizer is very cumbersome at the > moment. I had to override all concrete methods in the base class and > completely > duplicate the PageLock class because waitForRelease and markReleased have > package visibility. > > I would suggest to make these two methods in PageLock public so the class > can be reused and extract all the locking logic into an ILockManager with 3 > methods: lockPage, unlockPage, and unlockAllPages. The default lock > manager could hold the session-scoped locks collection and custom > implementations > could provide their own locking mechanism. > > I will create a JIRA issue for this. > > Best regards, > > Thomas > > On Wed, Feb 26, 2020 at 8:17 PM Thomas Heigl <tho...@umschalt.com> wrote: > >> Hi Sven, >> >> Thanks for the link but I'm not using asynchronous serialization. >> >> I thought some more about the issue and I think I figured it out. My >> setup looks like this: >> >> 1. Spring Session Redis >> 2. [Session Cache] (Not used because it is transient and stored with >> writeObject/readObject and does not get serialized into Redis as we do not >> use Java serialization) >> 3. PerSessionPageStore (Application-level cache held in memory) >> 4. RedisDataStore (Synchronous) >> >> Observations: >> >> 1. If i disable second-level cache or use the serializing second-level >> cache provided by the DefaultPageManager, there are no issues >> 2. As soon as I enable the PerSessionPageStore we run into concurrency >> issues >> >> Analysis: >> >> I first thought that there were some thread-safety issues >> with PerSessionPageStore but that is not the case because even a fully >> synchronized version shows these problems. >> >> The reason why disabling the 2nd-level cache or using a serializing >> variant works, is because they do not operate on the same *instance* of the >> page. Each thread gets their >> own instance because the page is deserialized before being accessed. >> >> PerSessionPageStore stores the page in memory without serializing it, >> thus all threads share the same instance. This is also the case when using >> the session cache or >> the session-based stores, but the PageAccessSynchronizer bound to the >> session takes care of ensuring that only single request can manipulate the >> page at any given time. >> >> So how does the synchronizer work? It keeps a Map<Integer, PageLock> in >> the session and checks whether the page is locked on every request. In a >> non-replicated >> environment this works as expected as the session object lives in the >> servlet container and is the same for each concurrent request. In my case, >> the session >> is not provided by the servlet container, but fetched from Redis by >> Spring Session on every request. So each concurrent thread has *their own >> version* of the session and >> the locks are *not shared between threads* because the session is only >> saved back to Redis after the request has finished. >> >> So the problematic flow looks like this >> >> 1. A request comes in, we fetch the session from Redis, the request >> acquires the session-scoped lock and starts processing >> 2. Before the request is finished, another request comes in, fetches the >> session from Redis, the lock map is empty because the state of request #1 >> has not been persisted to Redis >> 3. Now both requests can modify the page and we run into concurrency >> issues >> >> Summary: >> >> PageAccessSynchronizer does not work with Spring Session or other >> solutions that replace the servlet container session. >> >> Possible solutions: >> >> 1. We could ensure that session locks are updated in Redis immediately >> but that still leaves a couple of milliseconds for race conditions and adds >> a lot of overhead >> 2. We could use an application-scoped PageAccessSynchronizer. This solves >> the problem as long as sessions are sticky and all concurrent requests are >> sent to the same server. >> 3. If we want to use non-sticky session we could use Redis locks for >> implementing a global PageAccessSynchronizer >> >> I would like to go with solution #2 for now. The problem is >> that PageAccessSynchronizer is not an interface. >> >> Would it be possible to extract an interface so I can easily implement >> access synchronizers with different scopes? >> >> Best regards, >> >> Thomas >> >> >> >> >> >> On Wed, Feb 26, 2020 at 12:24 PM Sven Meier <s...@meiers.net> wrote: >> >>> Hi Thomas, >>> >>> Im wondering whether you're running into >>> https://issues.apache.org/jira/browse/WICKET-6702 >>> >>> I've been working on a solution to that problem, which is caused by >>> pages being asynchronously serialized while another request is already >>> coming in. >>> >>> Or maybe it is something different. >>> Could you create a quickstart? >>> >>> Sven >>> >>> Am 25. Februar 2020 22:12:46 MEZ schrieb Thomas Heigl < >>> tho...@umschalt.com>: >>> >Hi again, >>> > >>> >I investigated a bit and it does not seem to have anything to do with >>> >the >>> >PerSessionPageStore. I implemented a completely synchronized version of >>> >it >>> >and the problems still exist. >>> > >>> >If I switch to the default second-level cache that stores serialized >>> >pages >>> >in application scope, everything works as expected. Only the >>> >non-serialized >>> >pages in PerSessionPageStore seem to be affected by concurrent ajax >>> >modifications. >>> > >>> >What is the difference between keeping pages in the session and keeping >>> >the >>> >same pages in the PerSessionPageStore? Is there some additional locking >>> >done for pages in the session? >>> > >>> >Best, >>> > >>> >Thomas >>> > >>> >On Tue, Feb 25, 2020 at 8:25 PM Thomas Heigl <tho...@umschalt.com> >>> >wrote: >>> > >>> >> Hi all, >>> >> >>> >> I'm currently experimenting with PerSessionPageStore as a >>> >second-level >>> >> cache. We are moving our page store from memory (i.e. session) to >>> >Redis and >>> >> keeping 1-2 pages per session in memory speeds up ajax requests quite >>> >a bit >>> >> because network roundtrips and (de)serialization can be skipped for >>> >cached >>> >> pages. >>> >> >>> >> Our application is very ajax heavy (it is basically a single page >>> >> application with lots of lazy-loading). While rapidly clicking around >>> >and >>> >> firing as many parallel ajax requests as possible, I noticed that it >>> >is >>> >> quite easy to trigger exceptions that I have never seen before. >>> >> ConcurrentModificationExceptions during serialization, >>> >> MarkupNotFoundExceptions, exceptions about components already >>> >dequeuing etc. >>> >> >>> >> So I had a look at the implementation of PerSessionPageStore and >>> >noticed >>> >> that is does not do any kind of synchronization and does not use >>> >atomic >>> >> operations when updating the cache. It seems to me that the >>> >second-level >>> >> cache is not really usable in a concurrent ajax environment. >>> >> >>> >> I think that writing pages to the second level cache store should >>> >either >>> >> synchronize on sessionId+pageId or attempt to use atomic operations >>> >> provided by ConcurrentHashMap. >>> >> >>> >> Did anyone else ever run into these issues? The code >>> >> of PerSessionPageStore is quite complex because of soft references, >>> >> skip-list maps etc. so I'm not sure what the right approach to >>> >address >>> >> these problems would be. >>> >> >>> >> Best regards, >>> >> >>> >> Thomas >>> >> >>> >>
