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

Fix 5205: Handle ConcurrentModification and NPE from opening dialogs while Roun… #5302

Conversation

Sleet01
Copy link
Collaborator

@Sleet01 Sleet01 commented Mar 31, 2024

…d Report being handled

This should clear up some of the Exceptions reported by CasualJoker, and avoid those Exceptions breaking the game and/or sidelining Princess.

Root cause is that we are doing some weird manual tracking of open dialogs within the ClientGUI, which exposes us to ConcurrentModificationExceptions in the UI itself. These, if not handled, will propagate and break the game.
If caught, they don't cause any harm because we're removing unneeded dialogs from our list of elements, and then trying to notify the deleted dialog of the event signaling the completion of its work (usually, reinforcing from a MUL).

This fix also puts a try/catch around the contents of WAA.toDisplayableString() to avoid the case where it throws an NPE because the client instance can't dereference some information about the current target, likely due to the above bug (or because the main thread is in the context of another dialog while also trying to update the chat box or Round Report). Either way, this shouldn't break the game.

Testing:

  • Ran all 3 projects' unit tests.
  • Followed same steps observed in CasualJoker's game when the reported Exceptions occurred.
  • Observed Princess-vs-Princess fight for many turns following reinforcing from MUL or RAT generator, which previously would throw one of the above Exceptions and then have one or another Princess instance hang.

Close #5205
Close MegaMek/mekhq#3879

Copy link
Member

@SJuliez SJuliez left a comment

Choose a reason for hiding this comment

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

I'm not sure about these changes.

@@ -5358,7 +5358,12 @@ public static ToHitData processDefenderSPAs(ToHitData toHit, Entity ae, Entity t

@Override
public String toDisplayableString(Client client) {
return "attacking " + getTarget(client.getGame()).getDisplayName() + " with " + getEntity(client.getGame()).getEquipment(weaponId).getName();
try {
Copy link
Member

Choose a reason for hiding this comment

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

This will certainly prevent an NPE here. I'm still dubious. Won't it just crash later when it tries to find the target or the weapon? If this is only called from the boardview to display some info and we definitely want to prevent exceptions for display info then I'd put the try block in boardview.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I can tell, this NPE path only occurs while we're updating the Chat window and get interrupted by, for instance, the ConcurrentMod exception above. This method appears to only be called from AccessibilityWindow, so as long as we output some string it should be fine.

That said, I can make it work more like the version in AbstractAttackAction.java:

        final Targetable target = getTarget(client.getGame());
        return (target == null) ? "Attacking Null Target with id " + getTargetId()
                : "Attacking " + target.getDisplayName();

so at least the target's ID gets printed.

@kuronekochomusuke
Copy link
Collaborator

#3994

@HammerGS HammerGS merged commit 99fed07 into MegaMek:master Apr 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants