search

magefree / mage

Magic Another Game Engine
http://xmage.today
MIT License
1.91k stars 779 forks source link

Devs: Java -> streams -> forEach can broke the game (lost priority bug)? #6207

Open JayDi85 opened 4 years ago

JayDi85 commented 4 years ago

Example code from Whirlwind Denial:

game.getStack()
                .stream()
                .filter(Objects::nonNull)
                .forEachOrdered(stackObject -> {
                    Cost cost = new GenericManaCost(4);
                    if (cost.canPay(source, source.getSourceId(), source.getControllerId(), game)
                            && player.chooseUse(outcome, "Pay {4} to prevent " + stackObject.getIdName() + " from being countered?", source, game)
                            && cost.pay(source, game, source.getSourceId(), source.getControllerId(), false)) {
                        return;
                    }
                    stackObject.counter(source.getSourceId(), game);
                });
        return true;
    }

It uses a direct game objects list like Stack. If something can change it inside then it will be broken by ConcurrentModificationException (example with stack processing - if something add or remove from stack). It can be modified in the same code or by ApplyEffect, ProcessActions, game events and other “hidden” logic.

Recommends:

jeffwadsworth commented 4 years ago

Was this ever validated or not? It is a pretty important issue nowadays.

JayDi85 commented 4 years ago

I dodn't research it yet. Potentially broken code -- with native lock/wait calls. If game thread make object's lock (lock or synchronized call), but later it return to that locked object from stream's code -- that can freeze the game in infinite wait. Locks uses for server's thread and user's feedbacks. Added in 2024: See starting message with examples

In current server's logs I can find rare concurrent access/modification exceptions -- but it maybe something else. Example:

INFO  2020-01-23 07:01:54,832 GAME started f81229c0-e78c-4195-88e0-5c26452d682e [Standard] BruceWayne - computer         =>[CALL main-2993] TableController.startGame
Exception in thread "CALL main-3000" java.util.ConcurrentModificationException
        at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:909)
        at java.util.ArrayList$Itr.next(ArrayList.java:859)
        at mage.abilities.effects.ContinuousEffects.preventedByRuleModification(ContinuousEffects.java:735)
        at mage.game.GameState.replaceEvent(GameState.java:716)
        at mage.game.GameState.replaceEvent(GameState.java:712)
        at mage.game.GameImpl.replaceEvent(GameImpl.java:2639)
        at mage.players.PlayerImpl.canLose(PlayerImpl.java:2499)
        at mage.players.PlayerImpl.lost(PlayerImpl.java:2473)
        at mage.players.PlayerImpl.concede(PlayerImpl.java:2392)
        at mage.game.GameImpl.concede(GameImpl.java:1196)
        at mage.server.game.GameController.sendPlayerAction(GameController.java:529)
        at mage.server.game.GameManager.sendPlayerAction(GameManager.java:111)
        at mage.server.MageServerImpl.lambda$null$37(MageServerImpl.java:814)
        at java.util.Optional.ifPresent(Optional.java:159)
        at mage.server.MageServerImpl.lambda$sendPlayerAction$38(MageServerImpl.java:812)
        at mage.server.MageServerImpl.lambda$execute$67(MageServerImpl.java:1161)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)

P.S. It's only theory. added in 2024:that’s error example related to wrong call thread, see #11460

JayDi85 commented 4 years ago

As a preventive measure: try not use any user intercation code (pay/choose/etc) from streams (e.g. from forEach).

JayDi85 commented 1 year ago

[[Whirlwind Denial]] can cause ConcurrentModificationException error. Possible use case: stack contains multiple abilities and try to counter some of it.

java.util.ConcurrentModificationException
    at java.util.ArrayDeque$DeqSpliterator.forEachRemaining(ArrayDeque.java:957)
        at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482)
        at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472)
        at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151)
        at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174)
        at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.util.stream.ReferencePipeline.forEachOrdered(ReferencePipeline.java:423)
        at mage.cards.w.WhirlwindDenialEffect.apply(WhirlwindDenial.java:61)
        at mage.abilities.AbilityImpl.resolveMode(AbilityImpl.java:198)
        at mage.abilities.AbilityImpl.resolve(AbilityImpl.java:185)
        at mage.game.stack.Spell.resolve(Spell.java:244)
        at mage.game.GameImpl.resolve(GameImpl.java:1659)
        at mage.game.GameImpl.playPriority(GameImpl.java:1585)
        at mage.game.turn.Step.priority(Step.java:61)
        at mage.game.turn.Phase.playStep(Phase.java:184)
        at mage.game.turn.Phase.play(Phase.java:89)
        at mage.game.turn.Turn.play(Turn.java:125)
        at mage.game.GameImpl.playTurn(GameImpl.java:1058)
        at mage.game.GameImpl.play(GameImpl.java:968)
        at mage.game.GameImpl.start(GameImpl.java:944)
        at mage.server.game.GameWorker.call(GameWorker.java:34)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
github-actions[bot] commented 1 year ago

Whirlwind Denial - (Gatherer) (Scryfall) (EDHREC)

{2}{U} Instant For each spell and ability your opponents control, counter it unless its controller pays {4}.

xenohedron commented 5 months ago

Whirlwind Denial error root cause was fixed by #10460.

It's fine to use forEach on a stream as long as the source of the stream is not modified by your forEach code. Same as any other ConcurrentModificationException.

The main one to watch out for is game.getStack() which returns the real game object. So if you use that then you have to modify it in a separate step, after iterating over it.