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