-
Notifications
You must be signed in to change notification settings - Fork 167
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
Register resource immediately if HttpService is available #5064
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-osgi/src/main/java/com/vaadin/flow/osgi/support/VaadinResourceTrackerComponent.java, line 193 at r1 (raw file):
resource.getPath(), bundle); resourceToRegistration.put(serviceId, registration); if (httpService != null) {
Is there any chance that activate()
is invoked after registerResource
which will register the resource twice?
Or concurrently? I see there is no lock on individual resource register/unregister, but only on activate
/deactivate
.
(just asking, not blocking)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @bogdanudrescu and @denis-anisimov)
flow-osgi/src/main/java/com/vaadin/flow/osgi/support/VaadinResourceTrackerComponent.java, line 193 at r1 (raw file):
Previously, bogdanudrescu (Bogdan Udrescu) wrote…
Is there any chance that
activate()
is invoked afterregisterResource
which will register the resource twice?Or concurrently? I see there is no lock on individual resource register/unregister, but only on
activate
/deactivate
.(just asking, not blocking)
There is a chance.
There is no chance that httpService
is not null if activate
is not called yet.
The component won't be activated until httpService
gets its not null
value.
So if httpService
is not null
then activate
has been already called.
Concurrency is handled by the instances: both resourceToRegistration
and contributorToRegistrations
are instantiated as synchronized maps .
So every single method call is thread safe.
The explicit synchronization is done when there is a need to modify maps atomically (several actions needs to be executed atomically).
But in fact this is out of the scope of this PR since I didn't introduce any additional map modification in the PR. The new code only does something with http service instance.
@@ -64,22 +65,23 @@ | |||
private final String path; | |||
private final Bundle bundle; | |||
|
|||
private volatile HttpContext context; | |||
private final AtomicReference<HttpContext> context = new AtomicReference<HttpContext>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SonarQube analysis reported 2 issues Watch the comments in this conversation to review them. 1 extra issueNote: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
Superseded by #5081 |
This change is