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

CLDR-17014 Make ExtraPaths.getInstance synchronized #4317

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

btangmu
Copy link
Member

@btangmu btangmu commented Jan 28, 2025

-ConcurrentModificationException was thrown when ExtraPaths.getInstance called computeIfAbsent during cldr-generate-json.sh

-Unable to repro the exception before making this change; anyway, this should prevent it

CLDR-17014

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

-ConcurrentModificationException was thrown when ExtraPaths.getInstance called computeIfAbsent during cldr-generate-json.sh

-Unable to repro the exception before making this change; anyway, this should prevent it
@btangmu btangmu self-assigned this Jan 28, 2025
@btangmu btangmu requested a review from srl295 January 28, 2025 18:40
@btangmu
Copy link
Member Author

btangmu commented Jan 28, 2025

FYI the stack trace was as follows, called from Ldml2JsonConverter.mapPathsToSections

📂#######################  main #######################
📍 main (step 1/9)	[000%]:	 ⚙️ Beginning parallel process of 1,067 files
[WARNING] 
com.google.common.util.concurrent.UncheckedExecutionException: com.google.common.util.concurrent.UncheckedExecutionException: Bad path: //ldml/numbers/otherNumberingSystems/traditional
    at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0 (Native Method)
    at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance (NativeConstructorAccessorImpl.java:62)
    at jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance (DelegatingConstructorAccessorImpl.java:45)
    at java.lang.reflect.Constructor.newInstance (Constructor.java:490)
    at java.util.concurrent.ForkJoinTask.getThrowableException (ForkJoinTask.java:600)
    at java.util.concurrent.ForkJoinTask.reportException (ForkJoinTask.java:678)
    at java.util.concurrent.ForkJoinTask.invoke (ForkJoinTask.java:737)
    at java.util.stream.Nodes.collect (Nodes.java:336)
    at java.util.stream.ReferencePipeline.evaluateToNode (ReferencePipeline.java:109)
    at java.util.stream.AbstractPipeline.evaluate (AbstractPipeline.java:545)
    at java.util.stream.AbstractPipeline.evaluateToArrayNode (AbstractPipeline.java:260)
    at java.util.stream.ReferencePipeline.toArray (ReferencePipeline.java:517)
    at java.util.stream.ReferencePipeline.toArray (ReferencePipeline.java:523)
    at org.unicode.cldr.json.Ldml2JsonConverter.processDirectory (Ldml2JsonConverter.java:2288)
    at org.unicode.cldr.json.Ldml2JsonConverter.processType (Ldml2JsonConverter.java:337)
    at org.unicode.cldr.json.Ldml2JsonConverter.main (Ldml2JsonConverter.java:300)
    at org.codehaus.mojo.exec.ExecJavaMojo$1.run (ExecJavaMojo.java:254)
    at java.lang.Thread.run (Thread.java:829)
Caused by: com.google.common.util.concurrent.UncheckedExecutionException: Bad path: //ldml/numbers/otherNumberingSystems/traditional
    at org.unicode.cldr.util.CLDRFile.getStringValue (CLDRFile.java:612)
    at org.unicode.cldr.util.CLDRFile.getWinningValue (CLDRFile.java:2898)
    at org.unicode.cldr.json.Ldml2JsonConverter.mapPathsToSections (Ldml2JsonConverter.java:555)
    at org.unicode.cldr.json.Ldml2JsonConverter.lambda$processDirectory$20 (Ldml2JsonConverter.java:2258)
    at java.util.stream.ReferencePipeline$3$1.accept (ReferencePipeline.java:195)
    at java.util.HashMap$KeySpliterator.forEachRemaining (HashMap.java:1621)
    at java.util.stream.AbstractPipeline.copyInto (AbstractPipeline.java:484)
    at java.util.stream.AbstractPipeline.wrapAndCopyInto (AbstractPipeline.java:474)
    at java.util.stream.Nodes$CollectorTask.doLeaf (Nodes.java:2191)
    at java.util.stream.Nodes$CollectorTask.doLeaf (Nodes.java:2157)
    at java.util.stream.AbstractTask.compute (AbstractTask.java:327)
    at java.util.concurrent.CountedCompleter.exec (CountedCompleter.java:746)
    at java.util.concurrent.ForkJoinTask.doExec (ForkJoinTask.java:290)
    at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec (ForkJoinPool.java:1020)
    at java.util.concurrent.ForkJoinPool.scan (ForkJoinPool.java:1656)
    at java.util.concurrent.ForkJoinPool.runWorker (ForkJoinPool.java:1594)
    at java.util.concurrent.ForkJoinWorkerThread.run (ForkJoinWorkerThread.java:183)
Caused by: java.util.ConcurrentModificationException
    at java.util.HashMap.computeIfAbsent (HashMap.java:1135)
    at org.unicode.cldr.util.ExtraPaths.getInstance (ExtraPaths.java:16)
    at org.unicode.cldr.util.CLDRFile.getRawExtraPathsPrivate (CLDRFile.java:3156)
    at org.unicode.cldr.util.CLDRFile.getRawExtraPaths (CLDRFile.java:3122)
    at org.unicode.cldr.util.CLDRFile.getFallbackPath (CLDRFile.java:731)
    at org.unicode.cldr.util.CLDRFile.getStringValue (CLDRFile.java:591)
    at org.unicode.cldr.util.CLDRFile.getWinningValue (CLDRFile.java:2898)
    at org.unicode.cldr.json.Ldml2JsonConverter.mapPathsToSections (Ldml2JsonConverter.java:555)
    at org.unicode.cldr.json.Ldml2JsonConverter.lambda$processDirectory$20 (Ldml2JsonConverter.java:2258)
    at java.util.stream.ReferencePipeline$3$1.accept (ReferencePipeline.java:195)
    at java.util.HashMap$KeySpliterator.forEachRemaining (HashMap.java:1621)
    at java.util.stream.AbstractPipeline.copyInto (AbstractPipeline.java:484)
    at java.util.stream.AbstractPipeline.wrapAndCopyInto (AbstractPipeline.java:474)
    at java.util.stream.Nodes$CollectorTask.doLeaf (Nodes.java:2191)
    at java.util.stream.Nodes$CollectorTask.doLeaf (Nodes.java:2157)
    at java.util.stream.AbstractTask.compute (AbstractTask.java:327)
    at java.util.concurrent.CountedCompleter.exec (CountedCompleter.java:746)
    at java.util.concurrent.ForkJoinTask.doExec (ForkJoinTask.java:290)
    at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec (ForkJoinPool.java:1020)
    at java.util.concurrent.ForkJoinPool.scan (ForkJoinPool.java:1656)
    at java.util.concurrent.ForkJoinPool.runWorker (ForkJoinPool.java:1594)
    at java.util.concurrent.ForkJoinWorkerThread.run (ForkJoinWorkerThread.java:183)
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE

@btangmu
Copy link
Member Author

btangmu commented Jan 28, 2025

Alternatively, maybe could use ConcurrentHashMap...

@btangmu
Copy link
Member Author

btangmu commented Jan 29, 2025

Compare alternative #4321 which uses ConcurrentHashMap -- but fails

@btangmu btangmu requested a review from macchiati January 29, 2025 17:59
@macchiati
Copy link
Member

This looks very strange. The extra paths that are added here are all static. That is, they do not depend on any locale data, just on stuff like StandardCodes, which don't vary by locale, and don't vary during any Survey Tool or ConsoleCheck run.

Since they can all be built statically, once, it should be just reformulated to be Bill Pugh Singleton, which is thread-safe and avoids the synchronization overhead. @srl295

Copy link
Member

@macchiati macchiati left a comment

Choose a reason for hiding this comment

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

See ticket

@srl295
Copy link
Member

srl295 commented Jan 29, 2025

First of all, the code change itself here makes sense - the collision was on the hash, and this synchronizes the hash. So merging this PR might make sense to remove the collision immediately..

However, I could only find one call site, in CLDRFile:

        ExtraPaths.getInstance(NameType.LANGUAGE).append(toAddTo);
        ExtraPaths.getInstance(NameType.SCRIPT).append(toAddTo);

So instead of making ExtraPaths synchronized, we could just remove ExtraPaths.getInstance() and have a public static ExtraPaths.append(toAddTo) - that's internally singleton initialized.

Copy link
Member

@srl295 srl295 left a comment

Choose a reason for hiding this comment

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

@macchiati could we consider merging this simple change, since PRs are failing, and then refactor separately?

@macchiati
Copy link
Member

We can merge for now, but need to leave the ticket open for a fix.

@btangmu btangmu requested a review from macchiati January 31, 2025 18:41
@btangmu btangmu dismissed macchiati’s stale review January 31, 2025 18:45

see comments, OK to merge for now

@btangmu btangmu merged commit ef324bd into unicode-org:main Jan 31, 2025
13 checks passed
@btangmu btangmu deleted the t17014_f branch January 31, 2025 18:45
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.

3 participants