Fix for lights not updating after token move #3981
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Most of `ServerMessageHandler`'s messages are able to interact with the model - or even the zone renderer directly - which means they should be run on the Swing thread to avoid thread safety issues. As an added perk, now that these handlers are serialied, we can avoid the need for the explicit mutex that was used to guard access to a portion of the model. `ClientMessageHandler` has also been updated to run more of its methods on the Swing thread. Most of the messages were already handled this way, but there was a few that ought to run on the Swing thread but weren't.
Now that `ServerMessageHandler` and `ClientMessageHandler` are more careful about running on the Swing thread, they do not cause off-thread flushes of the `ZoneView`. There are still a few places where off-thread acesss is possible (e.g., an app action that runs on a background thread) but these are far more rare and less likely to cause issues. Also solving them all would be quite involved.
cwisniew
approved these changes
Apr 20, 2023
|
I reran the checks. Maven decided to work this time. |
The maven repo had a DNS issue which was causing havoc (not just for us, not just for GitHub either) and that's why it was failing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Identify the Bug or Feature request
Fixes #3979
Description of the Change
In PR #3952, I modifed the
ZoneView.flush()to run on the Swing thread viaSwingUtilites.invokeLater(). This kept theZoneViewconsistent with itself, but other components assumed they could update theZoneRendererimmediately after the call toflush()was done. By delaying the call usinginvokeLater(), theZoneRendererwould sometime update prior to theZoneViewflush being run, causing old state to be rendered until another update.This PR goes a step further towards guaranteeing that
ZoneView.flush()is only called on the Swing thread.ClientMessageHandlerandServerMessageHandlerhave been updated to run their message handlers on the Swing thread if they are responsible for updating the model or theZoneRenderer. This removes the common cases of the ZoneView being accessed off-thread. With that, we no longer useSwingUtilities.invokeLater()inside of theZoneView.flush()methods.There might still be other places where off-thread access could happen (e.g., app actions run on a background thread). but a complete solution to threading is beyond this PR.
Possible Drawbacks
Other timing assumptions might be violated somewhere.
Documentation Notes
N/A
Release Notes
N/A
This change is