Fix race condition in CardEdition.getCardInSet()

[fediazgon]

The getCardInSet() method uses lazy initialization for cardsInSetLookupMap without synchronization. When BoosterDraft.loadCustomDrafts() processes multiple draft files in parallel using CompletableFuture.supplyAsync(), multiple threads can call getCardInSet() on the same CardEdition instance simultaneously.

List<CompletableFuture<?>> futures = new ArrayList<>();                                                                                                                                                  
for (final String element : dList) {                                                                                                                                                                     
    if (element.endsWith(FILE_EXT)) {                                                                                                                                                                    
        futures.add(CompletableFuture.supplyAsync(()-> { // HERE                                                                                                                                                 
            final List<String> dfData = FileUtil.readFile(ForgeConstants.DRAFT_DIR + element);                                                                                                           
            queue.add(CustomLimited.parse(dfData, FModel.getDecks().getCubes()));                                                                                                                        
            return null;                                                                                                                                                                                 
        }).exceptionally(ex -> {                                                                                                                                                                         
            ex.printStackTrace();                                                                                                                                                                        
            return null;                                                                                                                                                                                 
        }));                                                                                                                                                                                             
    }                                                                                                                                                                                                    
}                                                                                                                                                                                                        
CompletableFuture.allOf(futuresArray).join(); 

This causes TreeMap internal structure corruption with errors like:

Caused by: java.lang.NullPointerException: Cannot read field "left" because "r" is null                                                                                                                 
    at java.base/java.util.TreeMap.rotateLeft(TreeMap.java:2578)                                                                                                                                        
    at java.base/java.util.TreeMap.fixAfterInsertion(TreeMap.java:2645)                                                                                                                                 
    ...                                                                                                                                                                                                 
    at forge.card.CardEdition.getCardInSet(CardEdition.java:437)                                                                                                                                        

The solution is to apply double-checked locking pattern.

Closes #9504.

[Hanmac]

i wonder if it would be better to have cardsInSetLookupMap be created in the constructor
using Stream and Multimaps::toMultimap

cardsInSetLookupMap = cardStream.collect(
  toMultimap(
    e -> e.name,
    e -> e,
    MultimapBuilder.treeKeys(String.CASE_INSENSITIVE_ORDER).arrayListValues()::build)
  )
);

And getAllCardsInSet() can just return cardsInSetLookupMap.values() ?

[fediazgon]

i wonder if it would be better to have cardsInSetLookupMap be created in the constructor
using Stream and Multimaps::toMultimap

Ok, I thought this lazy initialization was important but it seems, during card database initialization, every card iterates through every edition calling getCardInSet(). This means the lookup map gets initialized for all editions during startup anyway. Let's initialize it in the constructor then.

Fixed in 3454c22

[Hanmac]

@tool4ever @tehdiplomat what do you think?
or should that optimized more?

[Hanmac]

@fediazgon
i don't know if we should do this in this MR or a later one,
but the Constructors might be clean-up more

Currently, it has 3 Constuctors:

• the big one that takes ListMultimaps:
• the extra one that takes native Array
• and the one that takes Datum Values (and a native Array)

The first one is the one used in the Reader for reading the Edition files:

forge/forge-core/src/main/java/forge/card/CardEdition.java

Line 770 in 2b563fb

CardEdition res = new CardEdition(cardMap, tokenMap, customPrintSheetsToParse);

the second one isn't used external, but only by the third one

the third one is used for

• the Unknown one:
  

  forge/forge-core/src/main/java/forge/card/CardEdition.java

  Line 276 in 2b563fb

  public static final CardEdition UNKNOWN = new CardEdition("1990-01-01", UNKNOWN_CODE, "??", Type.UNKNOWN, UNKNOWN_SET_NAME, FoilType.NOT_SUPPORTED, new EditionEntry[]{});

• some user bucket:
  

  forge/forge-core/src/main/java/forge/card/CardEdition.java

  Lines 919 to 920 in 2b563fb

  	CardEdition customBucket = new CardEdition("2990-01-01", "USER", "USER",
  	Type.CUSTOM_SET, "USER", FoilType.NOT_SUPPORTED, new EditionEntry[]{});

Each one with an empty card list → that means, the third constructor can be updated to remove that unused extra Param, and might remove the second constructor too.

(i might try a smaller MR that can be merged faster)

[fediazgon]

I think having this one as its own change allows us to link this PR in the future in case there are other race conditions lying around as a way of documenting what to do. If we add to many changes it can be easy to miss the point of what the problem and the fix were. Also, this is currently a big issue for anyone that is playing custom cube drafts since, when I tested it, it was failing 1/4 times.

[fediazgon]

@Hanmac Let's merge yours and this one. We can also just merge this one since it includes both commits. We will just show as 2 contributors for the same (squashed) commit.