Load specifications for DocService lazily
#4491
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportBase: 74.05% // Head: 74.11% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #4491 +/- ##
============================================
+ Coverage 74.05% 74.11% +0.06%
- Complexity 18167 18183 +16
============================================
Files 1536 1537 +1
Lines 67378 67457 +79
Branches 8520 8530 +10
============================================
+ Hits 49894 49999 +105
+ Misses 13412 13387 -25
+ Partials 4072 4071 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
minwoox
reviewed
Oct 24, 2022
core/src/main/java/com/linecorp/armeria/server/docs/DocService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocService.java
Outdated
Show resolved
Hide resolved
ikhoon
reviewed
Oct 31, 2022
core/src/main/java/com/linecorp/armeria/server/docs/DocService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/docs/DocService.java
Outdated
Show resolved
Hide resolved
Comment on lines
264
to
266
| void triggerPreload(Executor executor) { | ||
| targetPaths.forEach(path -> load(path, executor)); | ||
| } |
There was a problem hiding this comment.
How about consolidating triggerPreload and updateServices? and it seems AtomicReference could be removed.
void triggerPreload(List<ServiceConfig> services, Executor executor) {
this.services = services;
targetPaths.forEach(path -> load(path, executor));
}
jrhee17
force-pushed
the
feature/lazy-load-docs
branch
from
a51ef1a
to
11c519a
Compare
jrhee17
force-pushed
the
feature/lazy-load-docs
branch
from
11c519a
to
06eebd1
Compare
minwoox
reviewed
Nov 4, 2022
| @@ -103,18 +110,17 @@ public final class DocService extends SimpleDecoratingHttpService { | |||
| spiNamedTypeInfoProviders); | |||
| } | |||
|
|
|||
| private static final ExecutorService DEFAULT_EXECUTOR = Executors.newSingleThreadScheduledExecutor( | |||
There was a problem hiding this comment.
newSingleThreadExecutor because we don't do scheduling?
Also how about just creating this executor where it's used because it's mostly used only once when the server is started?
There was a problem hiding this comment.
Also how about just creating this executor where it's used because it's mostly used only once when the server is started?
Not sure if this is what you meant, but scheduled the executor to be shut down after file loading has completed 🙏
core/src/main/java/com/linecorp/armeria/server/docs/DocService.java
Outdated
Show resolved
Hide resolved
jrhee17
force-pushed
the
feature/lazy-load-docs
branch
from
1e4e5d7
to
cab57dc
Compare
jrhee17
force-pushed
the
feature/lazy-load-docs
branch
from
cab57dc
to
722cbb7
Compare
minwoox
approved these changes
Nov 9, 2022
ikhoon
approved these changes
Nov 15, 2022
|
Thanks a lot, @jrhee17! 🙇 |
|
We're looking forward to this - excellent! |
minwoox
pushed a commit
that referenced
this pull request
Dec 13, 2022
Motivation:
Previously, `DocService` wasn't able to handle the changed services as a result of `reconfigure`.
```
sb = Server.builder()
.annotatedService("/hello", new HelloService())
.serviceUnder("/docs", DocService.builder().build())
...
sb.reconfigure(sb2 -> sb2.annotatedService("/hello2", new HelloService())
.serviceUnder("/docs", DocService.builder().build()))
```
- If a shared `DocService` is used, services wouldn't be updated and the old specification would be used.
- If a new `DocService` is added, appropriate callbacks wouldn't be called and `/specifications.json` would result in a 404.
https://github.com/line/armeria/blob/cae3b6da373101c0976e2d00f1f3cbb706d07e00/core/src/main/java/com/linecorp/armeria/server/docs/DocService.java#L217-L220
We may note that `Server#configs` is immutable, and that `Server#configs` is fixed at the timing when the `serviceAdded` callback is called.
https://github.com/line/armeria/blob/cae3b6da373101c0976e2d00f1f3cbb706d07e00/core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java#L246
Modifications:
- Change the timing when `DocService#specifications` is loaded to `DecoratingService#serviceAdded`.
Result:
- Users can use `DocService` even when `Server#reconfigure` is called.
Note: This PR contains changes from #4491 . Review this PR first 🙏
trustin
pushed a commit
that referenced
this pull request
Mar 27, 2023
… service (#4688) Motivation: We use thrift/grpc internal APIs when constructing `DocService`. Following #4491, these internal APIs are called concurrently with service logic. Due to a thrift bug, a deadlock can occur when `FieldMetaData.getStructMetaDataMap` is called for a thrift class while the class is initialized. https://issues.apache.org/jira/browse/THRIFT-5430 This PR attempts to circumvent this issue by pre-loading the class for affected versions (<= armeria-thrift 0.14) I've also double checked that other `*MetaData` implementations don't have a similar implementation. Modifications: - Add a `ThriftMetadataAccess` utility class - When `ThriftMetadataAccess` is instantiated, check if (<= armeria-thrift 0.14) is in the classpath - If the vulnerable artifact is loaded, then pre-instantiate thrift classes before `FieldMetaData.getStructMetaDataMap` is called from `DocService` Result: - A deadlock is no longer observed when loading `DocService` for thrift services where `<= armeria-thrift 0.14`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Following auto generation of request/response objects for
DocService, some users have reported that long loading times block server startup. We may loadDocServicespecifications lazily on startup with additional generalizations so that we can later also handle updates resulting fromDocService#reconfigure.Modifications:
SpecificationLoaderwhich loads resources lazily if needed.Result:
DocServicespecifications #4479