juanpablo-santos commented on code in PR #307:
URL: https://github.com/apache/jspwiki/pull/307#discussion_r1412879450
##########
jspwiki-event/src/main/java/org/apache/wiki/event/WikiEventManager.java:
##########
@@ -406,15 +429,15 @@ public Set< WikiEventListener > getWikiEventListeners() {
* @return true if the listener was added (i.e., it was not already in
the list and was added)
*/
public boolean addWikiEventListener( final WikiEventListener listener
) {
- synchronized( m_listenerList ) {
+ return Synchronizer.synchronize(wikiEventListenerLock, () -> {
Review Comment:
shouldn't `m_listener` reuse the same `Lock` through the entire class?
Different locks on different operations would imply they could execute
operations on `m_listenerList` at the same time
##########
jspwiki-main/src/main/java/org/apache/wiki/auth/authorize/DefaultGroupManager.java:
##########
@@ -367,14 +395,18 @@ protected String[] extractMembers( final String
memberLine ) {
/** {@inheritDoc} */
@Override
- public synchronized void addWikiEventListener( final WikiEventListener
listener ) {
- WikiEventManager.addWikiEventListener( this, listener );
+ public void addWikiEventListener( final WikiEventListener listener ) {
Review Comment:
should use the same lock as `removeWikiEventListener`
##########
jspwiki-main/src/main/java/org/apache/wiki/WatchDog.java:
##########
@@ -136,26 +154,26 @@ private static void scrub() {
* Can be used to enable the WatchDog. Will cause a new Thread to be
created, if none was existing previously.
*/
public void enable() {
- synchronized( WatchDog.class ) {
+ Synchronizer.synchronize(enableLock, () -> {
Review Comment:
same as before, same synchronized object should reuse the same Lock
throughout the class.
##########
jspwiki-main/src/main/java/org/apache/wiki/auth/authorize/DefaultGroupManager.java:
##########
@@ -152,13 +180,13 @@ public void initialize( final Engine engine, final
Properties props ) throws Wik
// Load all groups from the database into the cache
final Group[] groups = m_groupDatabase.groups();
- synchronized( m_groups ) {
Review Comment:
converting `m_groups` to `ConcurrentHashMap` would avoid the use of
`Synchronizer`
##########
jspwiki-event/src/main/java/org/apache/wiki/event/WikiEventManager.java:
##########
@@ -314,35 +337,35 @@ private Map< Object, WikiEventDelegate > getDelegates() {
* @param client the client Object, or alternately a Class reference
* @return the WikiEventDelegate.
*/
- private WikiEventDelegate getDelegateFor( final Object client ) {
- synchronized( m_delegates ) {
Review Comment:
again, converting to `ConcurrentHashMap` would avoid all the syncing..
##########
jspwiki-main/src/main/java/org/apache/wiki/plugin/PageViewPlugin.java:
##########
@@ -129,6 +134,20 @@ public class PageViewPlugin extends AbstractReferralPlugin
implements Plugin, In
/** Constant for storage interval in seconds. */
private static final int STORAGE_INTERVAL = 60;
+ /**
+ * 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:
better `private final ReentrantLock lock = new ReentrantLock();` to avoid to
declare the constructor?
##########
jspwiki-main/src/main/java/org/apache/wiki/search/LuceneSearchProvider.java:
##########
@@ -389,9 +407,9 @@ protected Document luceneIndexPage( final Page page, final
String text, final In
field = new Field( LUCENE_PAGE_KEYWORDS, page.getAttribute(
"keywords" ).toString(), TextField.TYPE_STORED );
doc.add( field );
}
- synchronized( writer ) {
+ Synchronizer.synchronize(lock, () -> {
Review Comment:
additional lock here?
##########
jspwiki-main/src/main/java/org/apache/wiki/WatchDog.java:
##########
@@ -186,10 +204,10 @@ public void enterState( final String state ) {
*/
public void enterState( final String state, final int
expectedCompletionTime ) {
LOG.debug( "{}: Entering state {}, expected completion in {} s",
m_watchable.getName(), state, expectedCompletionTime );
- synchronized( m_stateStack ) {
+ Synchronizer.synchronize(enterStateLock, () -> {
Review Comment:
same as before..
##########
jspwiki-main/src/main/java/org/apache/wiki/auth/SessionMonitor.java:
##########
@@ -235,8 +260,10 @@ public final Principal[] userPrincipals() {
* @param listener the event listener
* @since 2.4.75
*/
- public final synchronized void addWikiEventListener( final
WikiEventListener listener ) {
- WikiEventManager.addWikiEventListener( this, listener );
+ public final void addWikiEventListener( final WikiEventListener listener )
{
Review Comment:
same..
##########
jspwiki-main/src/main/java/org/apache/wiki/auth/SessionMonitor.java:
##########
@@ -169,9 +195,9 @@ public final Session find( final String sessionId ) {
private Session createGuestSessionFor( final String sessionId ) {
LOG.debug( "Session for session ID={}... not found. Creating
guestSession()", sessionId );
final Session wikiSession = Wiki.session().guest( m_engine );
- synchronized( m_sessions ) {
- m_sessions.put( sessionId, wikiSession );
- }
+ Synchronizer.synchronize(createGuestSessionForLock, () -> {
Review Comment:
same as others, reuse locks with the same object
##########
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:
as noted before, perhaps better to just `private final ReentrantLock lock =
new ReentrantLock();` to avoid declaring the constructor? Also, seems that more
than one lock is needed
##########
jspwiki-event/src/main/java/org/apache/wiki/event/WikiEventManager.java:
##########
@@ -238,32 +265,28 @@ public static Set<WikiEventListener>
getWikiEventListeners( final Object client
* @return true if the listener was found and removed.
*/
public static boolean removeWikiEventListener( final WikiEventListener
listener ) {
- boolean removed = false;
+ final AtomicBoolean removed = new AtomicBoolean(false);
// get the Map.entry object for the entire Map, then check match on
entry (listener)
final WikiEventManager mgr = getInstance();
final Map< Object, WikiEventDelegate > sources =
Collections.synchronizedMap( mgr.getDelegates() );
- synchronized( sources ) {
+ Synchronizer.synchronize(removeWikiEventListenerLock, () -> {
// get an iterator over the Map.Enty objects in the map
for( final Map.Entry< Object, WikiEventDelegate > entry :
sources.entrySet() ) {
// the entry value is the delegate
final WikiEventDelegate delegate = entry.getValue();
// now see if we can remove the listener from the delegate
(delegate may be null because this is a weak reference)
if( delegate != null && delegate.removeWikiEventListener(
listener ) ) {
- removed = true; // was removed
+ removed.set(true); // was removed
}
}
- }
- return removed;
+ });
+ return removed.get();
}
private void removeDelegates() {
- synchronized( m_delegates ) {
- m_delegates.clear();
- }
- synchronized( m_preloadCache ) {
- m_preloadCache.clear();
- }
+ Synchronizer.synchronize(delegatesLockLock, m_delegates::clear);
Review Comment:
`m_delegates` could be converted to a `ConcurrentHashMap`, avoiding the need
of using `Synchronizer.synchronize`. Also `m_preloadCache` is a `Vector`, hence
thread-safe, so there's no need of syncing.. don't know why it was
`synchronized` in first place, though.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
