juanpablo-santos commented on code in PR #307:
URL: https://github.com/apache/jspwiki/pull/307#discussion_r1354460996
##########
jspwiki-main/src/main/java/org/apache/wiki/WatchDog.java:
##########
@@ -302,15 +315,15 @@ private void dumpStackTraceForWatchable() {
*/
@Override
public String toString() {
- synchronized( m_stateStack ) {
+ return Synchronizer.synchronize(lock, () -> {
Review Comment:
unrelated to the PR itself, but `java.util.Stack` is thread-safe (extends
`Vector`) so not sure why this was synced on in the first place... Also,
perhaps it could be substituted with a `ConcurrentLinkedDeque`. Anyway, out of
PR's scope.
##########
jspwiki-event/src/main/java/org/apache/wiki/event/WikiEventManager.java:
##########
@@ -154,11 +168,13 @@ private WikiEventManager() {
* @return A shared instance of the WikiEventManager
*/
public static WikiEventManager getInstance() {
- if( c_instance == null ) {
- synchronized( WikiEventManager.class ) {
- return new WikiEventManager();
- // start up any post-instantiation services here
- }
+ if (c_instance == null) {
+ Synchronizer.synchronize(lock, () -> {
Review Comment:
seems that the `synchronized` blocks on this class do so against different
objects: `WikiEventManager.class`, `m_delegates`, `m_preloadCache`, etc., so it
would make more sense to use different locks on `Synchronizer.synchronize`
calls?
##########
jspwiki-main/src/main/java/org/apache/wiki/WikiEngine.java:
##########
@@ -138,6 +141,16 @@ public class WikiEngine implements Engine {
/** Stores WikiEngine's associated managers. */
protected final Map< Class< ? >, Object > managers = new
ConcurrentHashMap<>();
+ /**
+ * A lock used to ensure thread safety when accessing shared resources.
+ * This lock provides more flexibility and capabilities than the intrinsic
locking mechanism,
+ * such as the ability to attempt to acquire a lock with a timeout, or to
interrupt a thread
+ * waiting to acquire a lock.
+ *
+ * @see java.util.concurrent.locks.ReentrantLock
+ */
+ private static final ReentrantLock lock = new ReentrantLock();
Review Comment:
again, perhaps several locks are needed
##########
jspwiki-main/src/main/java/org/apache/wiki/auth/authorize/DefaultGroupManager.java:
##########
@@ -75,6 +77,20 @@ public class DefaultGroupManager implements GroupManager,
Authorizer, WikiEventL
/** Map with GroupPrincipals as keys, and Groups as values */
private final Map< Principal, Group > m_groups = new HashMap<>();
+ /**
+ * A lock used to ensure thread safety when accessing shared resources.
+ * This lock provides more flexibility and capabilities than the intrinsic
locking mechanism,
+ * such as the ability to attempt to acquire a lock with a timeout, or to
interrupt a thread
+ * waiting to acquire a lock.
+ *
+ * @see java.util.concurrent.locks.ReentrantLock
+ */
+ private final ReentrantLock lock;
Review Comment:
same as before, better with a couple of locks?
##########
jspwiki-event/pom.xml:
##########
@@ -62,5 +62,10 @@
<artifactId>junit-jupiter-engine</artifactId>
<scope>test</scope>
</dependency>
+
+ <dependency>
+ <groupId>org.apache.jspwiki</groupId>
+ <artifactId>jspwiki-util</artifactId>
Review Comment:
needs `<version>${project.version}</version>` and `groupId` can be replaced
with `${project.groupId}` (see last comment on main pom.xml file)
##########
pom.xml:
##########
@@ -458,6 +458,13 @@
<artifactId>javax.servlet-api</artifactId>
<version>${javax-servlet-api.version}</version>
</dependency>
+
+ <dependency>
+ <groupId>org.apache.jspwiki</groupId>
+ <artifactId>jspwiki-util</artifactId>
Review Comment:
this shouldn't be needed here, jspwiki modules should be referenced on other
modules using `groupId:artifactId:version` where `groupId` is always
`${project.groupId}` and version always is `${project.version}` (see
[this](https://github.com/apache/jspwiki/blob/master/jspwiki-main/pom.xml#L34-L36)
for an example)
##########
jspwiki-main/src/main/java/org/apache/wiki/auth/user/XMLUserDatabase.java:
##########
@@ -91,27 +95,59 @@ public class XMLUserDatabase extends AbstractUserDatabase {
private Document c_dom;
private File c_file;
+ /**
+ * A lock used to ensure thread safety when accessing shared resources.
+ * This lock provides more flexibility and capabilities than the intrinsic
locking mechanism,
+ * such as the ability to attempt to acquire a lock with a timeout, or to
interrupt a thread
+ * waiting to acquire a lock.
+ *
+ * @see java.util.concurrent.locks.ReentrantLock
+ */
+ private final ReentrantLock lock;
+
+ public XMLUserDatabase() {
+ lock = new ReentrantLock();
+ }
+
/** {@inheritDoc} */
- @Override
- public synchronized void deleteByLoginName( final String loginName )
throws WikiSecurityException {
- if( c_dom == null ) {
- throw new WikiSecurityException( "FATAL: database does not exist"
);
- }
+ public void deleteByLoginName(final String loginName) throws
WikiSecurityException {
+ final AtomicBoolean userDeleted = new AtomicBoolean(false);
+ final AtomicReference<WikiSecurityException> exceptionRef = new
AtomicReference<>();
- final NodeList users =
c_dom.getDocumentElement().getElementsByTagName( USER_TAG );
- for( int i = 0; i < users.getLength(); i++ ) {
- final Element user = ( Element )users.item( i );
- if( user.getAttribute( LOGIN_NAME ).equals( loginName ) ) {
- c_dom.getDocumentElement().removeChild( user );
+ Synchronizer.synchronize(lock, () -> {
+ try {
+ if (c_dom == null) {
+ throw new WikiSecurityException("FATAL: database does not
exist");
+ }
+
+ final NodeList users =
c_dom.getDocumentElement().getElementsByTagName(USER_TAG);
+ for (int i = 0; i < users.getLength(); i++) {
+ final Element user = (Element) users.item(i);
+ if (user.getAttribute(LOGIN_NAME).equals(loginName)) {
+ c_dom.getDocumentElement().removeChild(user);
- // Commit to disk
- saveDOM();
- return;
+ // Commit to disk
+ saveDOM();
+ userDeleted.set(true);
+ return;
+ }
+ }
+ } catch (WikiSecurityException e) {
+ exceptionRef.set(e);
Review Comment:
the eternal problem with lambdas, no checked exceptions... great way to deal
with it!
##########
jspwiki-main/src/main/java/org/apache/wiki/WatchDog.java:
##########
@@ -57,6 +59,16 @@ public final class WatchDog {
private static final Map< Integer, WeakReference< WatchDog > > c_kennel =
new ConcurrentHashMap<>();
private static WikiBackgroundThread c_watcherThread;
+ /**
+ * A lock used to ensure thread safety when accessing shared resources.
+ * This lock provides more flexibility and capabilities than the intrinsic
locking mechanism,
+ * such as the ability to attempt to acquire a lock with a timeout, or to
interrupt a thread
+ * waiting to acquire a lock.
+ *
+ * @see java.util.concurrent.locks.ReentrantLock
+ */
+ private final ReentrantLock lock;
Review Comment:
same as noted above, perhaps more locks are needed (?)
##########
jspwiki-main/src/main/java/org/apache/wiki/search/LuceneSearchProvider.java:
##########
@@ -133,6 +135,20 @@ public class LuceneSearchProvider implements
SearchProvider {
private static final String PUNCTUATION_TO_SPACES = StringUtils.repeat( "
", TextUtil.PUNCTUATION_CHARS_ALLOWED.length() );
+ /**
+ * A lock used to ensure thread safety when accessing shared resources.
+ * This lock provides more flexibility and capabilities than the intrinsic
locking mechanism,
+ * such as the ability to attempt to acquire a lock with a timeout, or to
interrupt a thread
+ * waiting to acquire a lock.
+ *
+ * @see java.util.concurrent.locks.ReentrantLock
+ */
+ private final ReentrantLock lock;
Review Comment:
more than one lock needed?
##########
jspwiki-main/src/main/java/org/apache/wiki/auth/SessionMonitor.java:
##########
@@ -59,6 +63,16 @@ public class SessionMonitor implements HttpSessionListener {
private final PrincipalComparator m_comparator = new PrincipalComparator();
+ /**
+ * A lock used to ensure thread safety when accessing shared resources.
+ * This lock provides more flexibility and capabilities than the intrinsic
locking mechanism,
+ * such as the ability to attempt to acquire a lock with a timeout, or to
interrupt a thread
+ * waiting to acquire a lock.
+ *
+ * @see ReentrantLock
+ */
+ private final ReentrantLock lock;
Review Comment:
syncs against `m_sessions` and at method level, so it should use two locks
(?)
##########
jspwiki-main/src/main/java/org/apache/wiki/plugin/PageViewPlugin.java:
##########
@@ -446,6 +468,8 @@ public String execute( final Context context, final Map<
String, String > params
// let the engine render the list
result = engine.getManager( RenderingManager.class
).textToHTML( context, buf.toString() );
Review Comment:
I guess this is one of the cases that could not be handled by
`Synchronizer.synchronize`? b/c of `result` not being effectively final? Would
ensuring result gets assigned only inside the lambda work? Something like
```
final String result;
Synchronizer.synchronize( lock, () -> {
[...]
if( show == null || STR_NONE.equals( show ) ) {
result =STR_EMPTY;
} else if( PARAM_COUNT.equals( show ) ) {
[...]
result = counter.toString();
} else if( body != null && 0 < body.length() && STR_LIST.equals( show )
) {
[...]
result = engine.getManager( RenderingManager.class ).textToHTML(
context, buf.toString() );
}
} );
```
If not it's ok to leave the code as it is, having to force the code into
`Synchronized.synchronized` for the sake of doing it makes no sense
##########
jspwiki-main/src/main/java/org/apache/wiki/references/DefaultReferenceManager.java:
##########
@@ -301,101 +323,107 @@ private String getHashFileName( final String pageName )
{
/**
* Reads the serialized data from the disk back to memory. Returns the
date when the data was last written on disk
*/
- private synchronized long unserializeAttrsFromDisk( final Page p ) throws
IOException, ClassNotFoundException {
- long saved = 0L;
+ private long unserializeAttrsFromDisk( final Page p ) throws IOException,
ClassNotFoundException {
+ lock.lock();
+ try {
+ long saved = 0L;
+
+ // Find attribute cache, and check if it exists
+ final String hashName = getHashFileName( p.getName() );
+ if( hashName != null ) {
+ File f = new File( m_engine.getWorkDir(), SERIALIZATION_DIR );
+ f = new File( f, hashName );
+ if( !f.exists() ) {
+ return 0L;
+ }
- // Find attribute cache, and check if it exists
- final String hashName = getHashFileName( p.getName() );
- if( hashName != null ) {
- File f = new File( m_engine.getWorkDir(), SERIALIZATION_DIR );
- f = new File( f, hashName );
- if( !f.exists() ) {
- return 0L;
- }
+ try( final ObjectInputStream in = new ObjectInputStream( new
BufferedInputStream( Files.newInputStream( f.toPath() ) ) ) ) {
+ final StopWatch sw = new StopWatch();
+ sw.start();
+ LOG.debug( "Deserializing attributes for {}", p.getName()
);
- try( final ObjectInputStream in = new ObjectInputStream( new
BufferedInputStream( Files.newInputStream( f.toPath() ) ) ) ) {
- final StopWatch sw = new StopWatch();
- sw.start();
- LOG.debug( "Deserializing attributes for {}", p.getName() );
+ final long ver = in.readLong();
+ if( ver != serialVersionUID ) {
+ LOG.debug( "File format has changed; cannot
deserialize." );
+ return 0L;
+ }
- final long ver = in.readLong();
- if( ver != serialVersionUID ) {
- LOG.debug( "File format has changed; cannot deserialize."
);
- return 0L;
- }
+ saved = in.readLong();
+ final String name = in.readUTF();
+ if( !name.equals( p.getName() ) ) {
+ LOG.debug( "File name does not match ({}),
skipping...", name );
+ return 0L; // Not here
+ }
- saved = in.readLong();
- final String name = in.readUTF();
- if( !name.equals( p.getName() ) ) {
- LOG.debug( "File name does not match ({}), skipping...",
name );
- return 0L; // Not here
- }
+ final long entries = in.readLong();
+ for( int i = 0; i < entries; i++ ) {
+ final String key = in.readUTF();
+ final Object value = in.readObject();
+ p.setAttribute( key, value );
+ LOG.debug( " attr: {}={}", key, value );
+ }
- final long entries = in.readLong();
- for( int i = 0; i < entries; i++ ) {
- final String key = in.readUTF();
- final Object value = in.readObject();
- p.setAttribute( key, value );
- LOG.debug( " attr: {}={}", key, value );
+ sw.stop();
+ LOG.debug( "Read serialized data for {} successfully in
{}", name, sw );
+ p.setHasMetadata();
}
-
- sw.stop();
- LOG.debug( "Read serialized data for {} successfully in {}",
name, sw );
- p.setHasMetadata();
}
- }
- return saved;
+ return saved;
+ } finally {
+ lock.unlock();
Review Comment:
the diff is really clobbed up here, why `Synchronizer` can't be used here?
Being a synchronized method, it shouldn't be a problem :-?
##########
jspwiki-main/src/main/java/org/apache/wiki/workflow/DefaultWorkflowManager.java:
##########
@@ -70,6 +82,7 @@ public DefaultWorkflowManager() {
m_approvers = new ConcurrentHashMap<>();
m_queue = new DecisionQueue();
WikiEventEmitter.attach( this );
+ // this.lock = new ReentrantLock();
Review Comment:
maybe delete this line?
##########
jspwiki-util/src/main/java/org/apache/wiki/util/Synchronizer.java:
##########
@@ -0,0 +1,150 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+package org.apache.wiki.util;
+
+import java.util.concurrent.locks.ReentrantLock;
+import java.util.function.Supplier;
+
+/**
+ * <h1>Synchronizer Utility Class</h1>
+ *
+ * <p>This utility class is designed to provide a simplified interface for
+ * executing code blocks in a synchronized manner using {@link ReentrantLock}.
+ * It aims to improve code readability and maintainability by abstracting
+ * common locking idioms.</p>
+ *
+ * <h2>Usage Example:</h2>
+ * <pre>
+ * {@code
+ * ReentrantLock lock = new ReentrantLock();
+ * String result = Synchronizer.synchronize(lock, () -> {
+ * // Your synchronized code here
+ * return "some result";
+ * });
+ * }
+ * </pre>
+ *
+ * @since 2.12.2
+ */
+public class Synchronizer {
+
+ /**
+ * Executes a given {@link Supplier} within a synchronized block managed by
+ * a {@link ReentrantLock}.
+ *
+ * <p>This method acquires the lock, executes the supplier's code, and then
+ * releases the lock. It is designed to replace the traditional lock
idiom:</p>
+ *
+ * <pre>
+ * {@code
+ * lock.lock();
+ * try {
+ * doSomething();
+ * } finally {
+ * lock.unlock();
+ * }
+ * }
+ * </pre>
+ *
+ * <p><strong>Parameters:</strong></p>
+ * <ul>
+ * <li>{@code lock} - The ReentrantLock to be used for
synchronization.</li>
+ * <li>{@code supplier} - The supplier whose code needs to be executed
within
+ * the synchronized block.</li>
+ * </ul>
+ *
+ * <p><strong>Returns:</strong></p>
+ * <p>The result produced by the supplier.</p>
+ *
+ * <p><strong>Throws:</strong></p>
+ * <p>This method propagates any exceptions thrown by the supplier's
code.</p>
+ *
+ * @param <T> The type of result produced by the supplier.
+ * @param lock The ReentrantLock to be used for synchronization.
+ * @param supplier The supplier to be executed within the synchronized
block.
+ * @return The result produced by the supplier.
+ */
+ public static <T> T synchronize(final ReentrantLock lock, final
Supplier<T> supplier) {
+ lock.lock();
+ try {
+ return supplier.get();
+ } finally {
+ lock.unlock();
+ }
+ }
+
+ /**
+ * <p>Functional interface for runnable tasks that can throw
exceptions.</p>
+ *
+ * @param <E> the type of exception that may be thrown
+ */
+ @FunctionalInterface
+ public interface ThrowingRunnable<E extends Exception> {
+ /**
+ * Executes the operation.
+ *
+ * @throws E if an exception occurs during the operation
+ */
+ void run() throws E;
+ }
+
+ /**
+ * <p>Throws the given exception as an unchecked exception.</p>
+ *
+ * @param <E> the type of exception to throw
+ * @param exception the exception to throw
+ * @throws E the thrown exception
+ */
+ @SuppressWarnings("unchecked")
+ private static <E extends Throwable> void throwAsUnchecked(final Exception
exception) throws E {
+ throw (E) exception;
+ }
+
+ /**
+ * <p>Executes a given {@link ThrowingRunnable} within a synchronized
block managed by
+ * a {@link ReentrantLock}.</p>
+ *
+ * <p><strong>Parameters:</strong></p>
+ * <ul>
+ * <li>{@code lock} - The ReentrantLock to be used for
synchronization.</li>
+ * <li>{@code throwingRunnable} - The ThrowingRunnable whose code
needs to be executed within
+ * the synchronized block.</li>
+ * </ul>
+ *
+ * <p><strong>Throws:</strong></p>
+ * <p>This method propagates any exceptions thrown by the
ThrowingRunnable's code.</p>
+ *
+ * @param <E> the type of exception that may be thrown
+ * @param lock the ReentrantLock to use for synchronization
+ * @param throwingRunnable the ThrowingRunnable to execute
+ */
+ public static <E extends Exception> void synchronize(final ReentrantLock
lock, final ThrowingRunnable<E> throwingRunnable) {
+ lock.lock();
+ try {
+ throwingRunnable.run();
Review Comment:
didn't see this initially, good catch!!
##########
jspwiki-util/src/main/java/org/apache/wiki/util/Synchronizer.java:
##########
@@ -0,0 +1,150 @@
+/*
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+ */
+
+package org.apache.wiki.util;
+
+import java.util.concurrent.locks.ReentrantLock;
+import java.util.function.Supplier;
+
+/**
+ * <h1>Synchronizer Utility Class</h1>
+ *
+ * <p>This utility class is designed to provide a simplified interface for
+ * executing code blocks in a synchronized manner using {@link ReentrantLock}.
+ * It aims to improve code readability and maintainability by abstracting
+ * common locking idioms.</p>
+ *
+ * <h2>Usage Example:</h2>
+ * <pre>
+ * {@code
+ * ReentrantLock lock = new ReentrantLock();
+ * String result = Synchronizer.synchronize(lock, () -> {
+ * // Your synchronized code here
+ * return "some result";
+ * });
+ * }
+ * </pre>
+ *
+ * @since 2.12.2
+ */
+public class Synchronizer {
+
+ /**
+ * Executes a given {@link Supplier} within a synchronized block managed by
+ * a {@link ReentrantLock}.
+ *
+ * <p>This method acquires the lock, executes the supplier's code, and then
+ * releases the lock. It is designed to replace the traditional lock
idiom:</p>
+ *
+ * <pre>
+ * {@code
+ * lock.lock();
+ * try {
+ * doSomething();
+ * } finally {
+ * lock.unlock();
+ * }
+ * }
Review Comment:
duplicated line, can be deleted?
--
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.
To unsubscribe, e-mail: dev-unsubscr...@jspwiki.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
