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
