Hi Murray, I agree, listeners should only appear on startup, if they appear later on then there's something wrong.
It occurs to me that, in this case, perhaps replacing the listener list with a CopyOnWriteArrayList would be enough to tackle all the synchronized code (and the need to wrap it around Synchronizer on the PR). There are only about 30 listeners, give or take, and they get instantiated/added on startup, so CopyOnWriteArrayList makes sense to me in this case. WDYT? Would you mind sending a PR with that? cheers, juan pablo El sáb, 2 dic 2023, 23:53, Murray Altheim <murra...@altheim.com> escribió: > Hi Juan Pablo, > > It's been many years since I wrote that event manager and I assume there've > been many edits since, but it occurs to me that the vast majority of > changes > are made to the engine when the wiki engine is first started up. Isn't that > the case? I guess there's also the possibility of instances of listeners > popping up from its use on page-based plugins. Hmm. > > In any case, I would think that we consider what the purposes of the locks > are for in the first case. Since the manager is a singleton, used by the > whole wiki engine, we'd not want it to be having its set of listeners > modified > while in the middle of processing an event. So I would tend to agree with > you that at least for 'm_listenerList' a single lock is best. On the other > hand, another approach would be to modify the event manager to be able to > simply ignore the issue, i.e., if an event comes through for a listener > that > is in the process of being added, I'm not sure what the harm is in ignoring > that. The chance of that event being tied to the new listener seems rather > remote. Dunno. We'd just need to avoid ConcurrentModificationExceptions. > > [I'm somewhat embarrassed to see that at that time (about 20 years ago) I > had gotten into the habit of using Hungarian Notation in my code. I suppose > when I was doing all my coding in vi it made more sense. Now it just looks > weird to me.] > > Cheers, > > Murray > > On 3/12/23 11:24, juanpablo-santos (via GitHub) wrote: > > > > 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. > > > > > > > > -- > > ........................................................................... > Murray Altheim <murray18 at altheim dot com> = = === > http://www.altheim.com/murray/ === > === > = = > === > In the evening > The rice leaves in the garden > Rustle in the autumn wind > That blows through my reed hut. > -- Minamoto no Tsunenobu > >
