perf(rest): inject ThriftClients as Spring bean across all REST services #3901
Open
Aman-Cool wants to merge 1 commit intoeclipse-sw360:mainfrom
Open
perf(rest): inject ThriftClients as Spring bean across all REST services #3901Aman-Cool wants to merge 1 commit intoeclipse-sw360:mainfrom
Aman-Cool wants to merge 1 commit intoeclipse-sw360:mainfrom
Conversation
…lasses Replaces per-call new ThriftClients() construction in all 22 REST service classes with a single injected Spring bean declared in Sw360ResourceServer. Step 1 of Thrift removal migration — no functional changes.
Contributor
Author
|
@GMishx @amritkv, This picks up where #3849 left off, that PR consolidated one service to a shared field, this one promotes it to a proper Spring bean and rolls the same change across all 22 service classes. The diff looks big but it's the same 3-6 line change repeated: add the injected field, delete the local vars. No logic touched anywhere. Main thing worth a second look is |
This file contains hidden or 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
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.
perf(rest): centralize ThriftClients as a Spring bean across all REST services
What's happening
Every REST service class in
rest/resource-serverwas constructing a newThriftClientsobject at the point of use; inline expressions, per-method local variables, or field initializers that bypassed Spring entirely.Sw360ProjectServicealone had 19 separatenew ThriftClients()declarations across its methods.ThriftClientsis not lightweight, each instantiation reads connection config and sets up HTTP transport. More critically, instances usingTHttpClientinternally open OS sockets that were never explicitly closed, causing the file descriptor leak documented in #3849 and #3760.This PR declares
ThriftClientsas a single@BeaninSw360ResourceServerand injects it into all 22 service classes via@RequiredArgsConstructor. The only remainingnew ThriftClients()in the module is the factory method itself.Prior work
ProjectHandlerandSw360ClearingRequestService. Fix/clearing request thrift client leak #3849 established the shared-field pattern this PR extends to the whole module.Files changed (22)
Most files: add
private final ThriftClients thriftClients;field, remove local vars / inline constructions, update call sites. 3-6 line change each.Special case-
SW360ReportService: had four eagerly-initialized Thrift client fields (new ThriftClients().makeProjectClient()etc.) at field declaration time. Field initializers run before the constructor, so they can't reference an injected field, these were removed and call sites now callthriftClients.makeXxxClient()directly, consistent with every other service in the module.Excluded-
Sw360UserService: receivesThriftClientsas a method parameter in two static utility methods, different pattern, out of scope.Migration roadmap
This is Step 1 of the [GSoC 2026 Thrift removal project]:
ThriftClientsinjections for direct@Servicehandler beanslibthriftdependencyStep 2 is easier now: injection seams are already in place across all services.
Reviewer notes
@RequiredArgsConstructoradded toSW360RestHealthIndicatorandSw360UserDetailsProvider(the only two that didn't have it)Issue: NA
Suggest Reviewers:
@GMishx @amritkv @rudra-superrr @bibhuti230185
Checklist
Must: