Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add settings change notifier #8813

Merged
merged 2 commits into from
Feb 13, 2025

Conversation

wmontwe
Copy link
Member

@wmontwe wmontwe commented Feb 11, 2025

This introduces a new settings change notification system to the project. The main changes include adding a publish/subscribe pattern with a broker and publisher for settings changes, updating existing classes to use this new system, and adding tests for the new functionality.

The drawer is now immediatelly updated whenever a setting is changed.

Resolves #8765

Copy link
Member

@cketti cketti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a race condition in DefaultSettingsChangeBroker.

Comment on lines 22 to 26
for (subscriber in subscribers) {
subscriber.receive()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This accesses subscribers without synchronizing on lock. If a subscriber is removed in one thread while this is iterating over subscribers in another thread, a ConcurrentModificationException could be thrown.

The problem can be avoided by also wrapping this method body in synchronized(lock) { … } or by using CopyOnWriteArraySet which can be used without any manual locking/synchronization.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I avoided synchronizing this method to balance performance with the low likelihood of concurrent modifications in our use case. However, I agree that the current approach is not robust and should be replaced with a more comprehensive solution in the future.

The CopyOnWriteArraySet is a Java only dependency and I opted for a Kotlin specific alternative.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a lock around the receivers to avoid ConcurrentModificationException

@wmontwe wmontwe force-pushed the add-settings-change-notifier branch from 1258b94 to 84c7678 Compare February 13, 2025 14:47
@wmontwe wmontwe requested a review from cketti February 13, 2025 14:48
@wmontwe wmontwe merged commit 73628c1 into thunderbird:main Feb 13, 2025
3 checks passed
@wmontwe wmontwe deleted the add-settings-change-notifier branch February 13, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show/Hide star count does not update immediately
2 participants