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 concurrent problems when initializing multiple GroupedOpenApi parallelly #1707

Merged
merged 1 commit into from Jun 20, 2022

Conversation

bianjp
Copy link

@bianjp bianjp commented Jun 14, 2022

Problem

Continuation of #1641

If a project has multiple GroupedOpenApi bean, they will be initialized parallelly. This lead to concurrent read and write of AbstractOpenApiResource#HIDDEN_REST_CONTROLLERS.

Since the list is not correctly synchronized, concurrent read and write might cause ConcurrentModificationException (Collections#synchronizedList isn't thread-safe on iteration):

Exception in thread "pool-3-thread-1" java.util.ConcurrentModificationException
  at java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1363)
  at java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:126)
  at java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:499)
  at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:486)
  at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472)
  at java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:230)
  at java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:196)
  at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
  at java.util.stream.ReferencePipeline.anyMatch(ReferencePipeline.java:516)
  at org.springdoc.api.AbstractOpenApiResource.isHiddenRestControllers(AbstractOpenApiResource.java:822)
  at org.springdoc.api.AbstractOpenApiResource.lambda$getOpenApi$3(AbstractOpenApiResource.java:309)
  at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:174)
  at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:175)
  at java.util.HashMap$EntrySpliterator.forEachRemaining(HashMap.java:1723)
  at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482)
  at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472)
  at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
  at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
  at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:566)
  at org.springdoc.api.AbstractOpenApiResource.getOpenApi(AbstractOpenApiResource.java:310)
  at org.springdoc.api.AbstractOpenApiResource.getOpenApi(AbstractOpenApiResource.java:256)
  at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
  at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
  at java.lang.Thread.run(Thread.java:750)

Solution

There are 3 possible solutions:

  1. Synchronize on the list before every read and write:

    synchronized (HIDDEN_REST_CONTROLLERS) {
        return HIDDEN_REST_CONTROLLERS.stream().anyMatch(clazz -> clazz.isAssignableFrom(rawClass));
    }
    • Cons: The list is much more often used for read than write (in my project it's 1698 to 12), so synchronize on every read might hurt performance.
  2. Use an fully thread-safe implementation for list, such as CopyOnWriteArrayList

    • Pros: Read operations don't need synchronization
    • Cons: Write operations are very expensive since they usually entail copying the entire underlying array. However, each GroupedOpenApi only triggers two write operations for HIDDEN_REST_CONTROLLERS, and the list is generally small, so the trade-off is acceptable.
  3. Reactor the initialization to eliminate concurrent write
    Each GroupedOpenApi will try to add the same controllers to HIDDEN_REST_CONTROLLERS in OpenAPIService#initializeHiddenRestController, since the controllers are obtained from the same ApplicationContext.
    It's better to write to HIDDEN_REST_CONTROLLERS only once before parallel initialization. This can not only eliminate concurrent write but also avoid duplication.
    However, this refactoring requires more familiarity of Springdoc's code base, and it's beyond my powers at present.

This PR implements solution 2.

I also replaced ADDITIONAL_REST_CONTROLLERS, HIDDEN_REST_CONTROLLERS with set to avoid duplication. This will reduce the number of elements to iterate thus improving performance.

@bnasslahsen bnasslahsen merged commit ab008ac into springdoc:master Jun 20, 2022
@bnasslahsen
Copy link
Contributor

Thank you @bianjp for your contribution to the project.

@bianjp
Copy link
Author

bianjp commented Jul 27, 2022

@bnasslahsen Can you release a new version?

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.

None yet

2 participants