You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by GitBox <gi...@apache.org> on 2020/02/28 12:08:03 UTC
[GitHub] [wicket] theigl opened a new pull request #411: Wicket 6751 Page
synchronizer strategies
theigl opened a new pull request #411: Wicket 6751 Page synchronizer strategies
URL: https://github.com/apache/wicket/pull/411
This PR adds support for providing a custom lock manager for page access synchronization.
Session-scoped locking only works in servlet container environments where the session is a global object but does not work with Spring Session where the session is (de)serialized to external storage on every request.
For a detailed discussion of the subject see the [mailing list](http://mail-archives.apache.org/mod_mbox/wicket-users/202002.mbox/%3cCANtfzo5+YTBFzyP-U_dVjSrRu8xgv1qj4ozAWxDAbwVnF5=M9Q@mail.gmail.com%3e).
The following changes were made:
- Expose the currently package scoped methods for locking and waiting on a lock of `PageLock` so the class can be re-used in custom implementations
- Encapsulate all logic related to locking to an `IPageLockManager` and move the current locking implementation to `DefaultPageLockManager`
All changes are backwards-compatible from an API perspective. The only method that was moved is the `getTimeout(int pageId)` method on `PageAccessSynchronizer` that was public for some reason. I could add the method to the `IPageLockManager` interface and restore it as a delegate in `PageAccessSynchronizer` if wanted.
See https://issues.apache.org/jira/projects/WICKET/issues/WICKET-6751
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] martin-g commented on a change in pull request #411:
WICKET-6751 Page synchronizer strategies
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #411: WICKET-6751 Page synchronizer strategies
URL: https://github.com/apache/wicket/pull/411#discussion_r388176309
##########
File path: wicket-core/src/main/java/org/apache/wicket/page/DefaultPageLockManager.java
##########
@@ -0,0 +1,201 @@
+package org.apache.wicket.page;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.time.Instant;
+import java.util.Iterator;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Supplier;
+
+import org.apache.wicket.Application;
+import org.apache.wicket.settings.ExceptionSettings;
+import org.apache.wicket.util.LazyInitializer;
+import org.apache.wicket.util.lang.Threads;
+import org.apache.wicket.util.time.Durations;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default {@link IPageLockManager} that that holds a map of locks in the current session.
+ *
+ * @author Igor Vaynberg (ivaynberg)
+ */
+public class DefaultPageLockManager implements IPageLockManager, Serializable {
+
+ private static final long serialVersionUID = 1L;
+
+ private static final Logger logger = LoggerFactory.getLogger(DefaultPageLockManager.class);
+
+ /** map of which pages are owned by which threads */
+ private final Supplier<ConcurrentMap<Integer, PageAccessSynchronizer.PageLock>> locks = new LazyInitializer<ConcurrentMap<Integer, PageAccessSynchronizer.PageLock>>()
+ {
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ protected ConcurrentMap<Integer, PageAccessSynchronizer.PageLock> createInstance()
+ {
+ return new ConcurrentHashMap<>();
+ }
+ };
+
+ /** timeout value for acquiring a page lock */
+ private final Duration timeout;
+
+ /**
+ * Constructor
+ *
+ * @param timeout
+ * timeout value for acquiring a page lock
+ */
+ public DefaultPageLockManager(Duration timeout)
+ {
+ this.timeout = timeout;
+ }
+
+ private static long remaining(Instant start, Duration timeout)
+ {
+ Duration elapsedTime = Durations.elapsedSince(start);
+ return Math.max(0, timeout.minus(elapsedTime).toMillis());
+ }
+
+ /**
+ * @param pageId
+ * the id of the page to be locked
+ * @return the duration for acquiring a page lock
+ */
+ public Duration getTimeout(int pageId)
+ {
+ return timeout;
+ }
+
+ @Override
+ public void lockPage(int pageId) throws CouldNotLockPageException
+ {
+ final Thread thread = Thread.currentThread();
+ final PageAccessSynchronizer.PageLock lock = new PageAccessSynchronizer.PageLock(pageId, thread);
+ final Instant start = Instant.now();
+
+ boolean locked = false;
+
+ final boolean isDebugEnabled = logger.isDebugEnabled();
+
+ PageAccessSynchronizer.PageLock previous = null;
+
+ Duration timeout = getTimeout(pageId);
+
+ while (!locked && Durations.elapsedSince(start).compareTo(timeout) < 0)
+ {
+ if (isDebugEnabled)
+ {
+ logger.debug("'{}' attempting to acquire lock to page with id '{}'",
+ thread.getName(), pageId);
+ }
+
+ previous = locks.get().putIfAbsent(pageId, lock);
+
+ if (previous == null || previous.getThread() == thread)
+ {
+ // first thread to acquire lock or lock is already owned by this thread
+ locked = true;
+ }
+ else
+ {
+ // wait for a lock to become available
+ long remaining = remaining(start, timeout);
+ if (remaining > 0)
+ {
+ previous.waitForRelease(remaining, isDebugEnabled);
+ }
+ }
+ }
+ if (locked)
Review comment:
please add an empty line before line 112
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] svenmeier commented on issue #411: WICKET-6751 Page
synchronizer strategies
Posted by GitBox <gi...@apache.org>.
svenmeier commented on issue #411: WICKET-6751 Page synchronizer strategies
URL: https://github.com/apache/wicket/pull/411#issuecomment-592550517
If PageAccessSynchronizer delegates everything to a IPageLockManager anyway, wouldn't it be simpler to introduce IPageAccessSynchronizer instead? Why add another redirection?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] theigl commented on issue #411: WICKET-6751 Page
synchronizer strategies
Posted by GitBox <gi...@apache.org>.
theigl commented on issue #411: WICKET-6751 Page synchronizer strategies
URL: https://github.com/apache/wicket/pull/411#issuecomment-595135207
@martin-g: Thanks for reviewing! I'll push the requested changes asap.
What do you think about this issue from my PR description:
> I'm not sure if PageLock is in the right place now. It is still a static inner class of PageAccessSynchronizer but it is only used by DefaultLockManager and can be used by custom lock manager implementations. It would probably be better to make it a top-level class.
Should we keep it in `PageAccessSynchronizer` even though it is not used by the class anymore? Instead of making it a top-level class, I could also move it to `DefaultLockManager`. This probably makes the most sense.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] martin-g merged pull request #411: WICKET-6751 Page
synchronizer strategies
Posted by GitBox <gi...@apache.org>.
martin-g merged pull request #411: WICKET-6751 Page synchronizer strategies
URL: https://github.com/apache/wicket/pull/411
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] martin-g commented on a change in pull request #411:
WICKET-6751 Page synchronizer strategies
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #411: WICKET-6751 Page synchronizer strategies
URL: https://github.com/apache/wicket/pull/411#discussion_r388174068
##########
File path: wicket-core/src/main/java/org/apache/wicket/page/DefaultPageLockManager.java
##########
@@ -0,0 +1,201 @@
+package org.apache.wicket.page;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.time.Instant;
+import java.util.Iterator;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Supplier;
+
+import org.apache.wicket.Application;
+import org.apache.wicket.settings.ExceptionSettings;
+import org.apache.wicket.util.LazyInitializer;
+import org.apache.wicket.util.lang.Threads;
+import org.apache.wicket.util.time.Durations;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default {@link IPageLockManager} that that holds a map of locks in the current session.
+ *
+ * @author Igor Vaynberg (ivaynberg)
Review comment:
Please remove this line. It is not really true anymore.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] theigl commented on issue #411: WICKET-6751 Page
synchronizer strategies
Posted by GitBox <gi...@apache.org>.
theigl commented on issue #411: WICKET-6751 Page synchronizer strategies
URL: https://github.com/apache/wicket/pull/411#issuecomment-601627779
@svenmeier : Hi. Since Martin already approved this, could you please take another look? I'd really like to see this merged at some point.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] theigl commented on issue #411: WICKET-6751 Page
synchronizer strategies
Posted by GitBox <gi...@apache.org>.
theigl commented on issue #411: WICKET-6751 Page synchronizer strategies
URL: https://github.com/apache/wicket/pull/411#issuecomment-602023465
@svenmeier: Thanks for your approval. I know this does not have high priority, I just wanted to make sure that this PR is in a mergeable state.
As you rightly pointed out, it is already possible to provide my own synchronizer and I have successfully done so, but it forced me to copy a lot of quite complex code to my project (among others the whole `PageLock` class). Since Spring Session is quite a well known project and others might run into this issue as well, I'd still vote for merging this. It will save the next person who runs into this a lot of work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] theigl edited a comment on issue #411: WICKET-6751 Page
synchronizer strategies
Posted by GitBox <gi...@apache.org>.
theigl edited a comment on issue #411: WICKET-6751 Page synchronizer strategies
URL: https://github.com/apache/wicket/pull/411#issuecomment-592555627
@svenmeier: This would be possible. But the only method of this interface would be:
`IPageManager adapt(IPageManager pagemanager)`
This is the only method ever called by Wicket. All other methods related to locking are internal. `PageAccessSynchronizer` still serves its purpose because it contains the logic for decorating the page manager and coordinating the locking and unlocking via the new `IPageLockManager`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] theigl commented on issue #411: WICKET-6751 Page
synchronizer strategies
Posted by GitBox <gi...@apache.org>.
theigl commented on issue #411: WICKET-6751 Page synchronizer strategies
URL: https://github.com/apache/wicket/pull/411#issuecomment-592555627
@svenmeier: This would be possible. But the only method of this interface would be:
`IPageManager adapt(IPageManager pagemanager)`
This is the only method every called by Wicket. All other methods related to locking are internal. `PageAccessSynchronizer` still serves its purpose because it contains the logic for decorating the page manager and coordinating the locking and unlocking via the new `IPageLockManager`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] theigl commented on a change in pull request #411:
WICKET-6751 Page synchronizer strategies
Posted by GitBox <gi...@apache.org>.
theigl commented on a change in pull request #411: WICKET-6751 Page synchronizer strategies
URL: https://github.com/apache/wicket/pull/411#discussion_r388195392
##########
File path: wicket-core/src/main/java/org/apache/wicket/page/DefaultPageLockManager.java
##########
@@ -0,0 +1,201 @@
+package org.apache.wicket.page;
+
+import java.io.Serializable;
+import java.time.Duration;
+import java.time.Instant;
+import java.util.Iterator;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Supplier;
+
+import org.apache.wicket.Application;
+import org.apache.wicket.settings.ExceptionSettings;
+import org.apache.wicket.util.LazyInitializer;
+import org.apache.wicket.util.lang.Threads;
+import org.apache.wicket.util.time.Durations;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Default {@link IPageLockManager} that that holds a map of locks in the current session.
+ *
+ * @author Igor Vaynberg (ivaynberg)
+ */
+public class DefaultPageLockManager implements IPageLockManager, Serializable {
+
+ private static final long serialVersionUID = 1L;
+
+ private static final Logger logger = LoggerFactory.getLogger(DefaultPageLockManager.class);
+
+ /** map of which pages are owned by which threads */
+ private final Supplier<ConcurrentMap<Integer, PageAccessSynchronizer.PageLock>> locks = new LazyInitializer<ConcurrentMap<Integer, PageAccessSynchronizer.PageLock>>()
+ {
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ protected ConcurrentMap<Integer, PageAccessSynchronizer.PageLock> createInstance()
+ {
+ return new ConcurrentHashMap<>();
+ }
+ };
+
+ /** timeout value for acquiring a page lock */
+ private final Duration timeout;
+
+ /**
+ * Constructor
+ *
+ * @param timeout
+ * timeout value for acquiring a page lock
+ */
+ public DefaultPageLockManager(Duration timeout)
+ {
+ this.timeout = timeout;
+ }
+
+ private static long remaining(Instant start, Duration timeout)
+ {
+ Duration elapsedTime = Durations.elapsedSince(start);
+ return Math.max(0, timeout.minus(elapsedTime).toMillis());
+ }
+
+ /**
+ * @param pageId
+ * the id of the page to be locked
+ * @return the duration for acquiring a page lock
+ */
+ public Duration getTimeout(int pageId)
+ {
+ return timeout;
+ }
+
+ @Override
+ public void lockPage(int pageId) throws CouldNotLockPageException
+ {
+ final Thread thread = Thread.currentThread();
+ final PageAccessSynchronizer.PageLock lock = new PageAccessSynchronizer.PageLock(pageId, thread);
+ final Instant start = Instant.now();
+
+ boolean locked = false;
+
+ final boolean isDebugEnabled = logger.isDebugEnabled();
+
+ PageAccessSynchronizer.PageLock previous = null;
+
+ Duration timeout = getTimeout(pageId);
+
+ while (!locked && Durations.elapsedSince(start).compareTo(timeout) < 0)
+ {
+ if (isDebugEnabled)
+ {
+ logger.debug("'{}' attempting to acquire lock to page with id '{}'",
+ thread.getName(), pageId);
+ }
+
+ previous = locks.get().putIfAbsent(pageId, lock);
+
+ if (previous == null || previous.getThread() == thread)
+ {
+ // first thread to acquire lock or lock is already owned by this thread
+ locked = true;
+ }
+ else
+ {
+ // wait for a lock to become available
+ long remaining = remaining(start, timeout);
+ if (remaining > 0)
+ {
+ previous.waitForRelease(remaining, isDebugEnabled);
+ }
+ }
+ }
+ if (locked)
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] theigl commented on issue #411: WICKET-6751 Page
synchronizer strategies
Posted by GitBox <gi...@apache.org>.
theigl commented on issue #411: WICKET-6751 Page synchronizer strategies
URL: https://github.com/apache/wicket/pull/411#issuecomment-593832036
@svenmeier: Do you need some additional information for reviewing this PR? I'm happy to provide it and work on the PR some more if changes are requested.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services