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

Adds an afterStart pseudo-event to LoomServer observable by Router implementations #9655

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ljnelson
Copy link
Member

This PR adds an afterStart "event" in the same spirit as the existing beforeStart and afterStop "events" that are found in ServerLifecycle implementations (but dispatchable only via Router implementations).

I am not sure that I have written it correctly and expect it to be discussed thoroughly. My goal with this first pass has been to simply preserve the spirit and style of the preexisting "event" notifications in LoomServer, together with preserving design decisions made internally by members of the Helidon team.

LoomServer (package private) has a collection of (package private) ServerListener instances, indexed by name, that have access to Router implementations internally. I found that to preserve the spirit of the existing code I had to provide access to these Routers, since they are currently the only recipients of beforeStart and afterStop "events", and so, it seemed to me, should be the recipients of (the new) afterStart "event".

Internal discussions said that the notification mechanism should be serial, i.e. not in parallel, so I did not use the parallel method to dispatch the afterStart "event".

Currently if an afterStart "observer" throws an exception undefined behavior occurs. I believe that there will be many good opinions on what should happen instead.

Internal discussions also said that the new "event" payload should be the WebServer itself so there is now a bidirectional dependency between, e.g., Router and WebServer. I'm not sure if this was intended to be the case.

I also noticed that Router is effectively implementing the ServerLifecycle contract, but does not officially implement it, so I didn't add that.

I expect that there will be many changes requested, possibly even a completely different architecture for this originally-intended-to-be-small change.

@ljnelson ljnelson added webserver 4.x Version 4.x labels Jan 16, 2025
@ljnelson ljnelson added this to the 4.2.0 milestone Jan 16, 2025
@ljnelson ljnelson self-assigned this Jan 16, 2025
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jan 16, 2025
@ljnelson ljnelson requested a review from tjquinno January 16, 2025 04:46
Signed-off-by: Laird Nelson <[email protected]>
@tjquinno
Copy link
Member

tjquinno commented Jan 25, 2025

I share what I think is your concern, Laird: currently, routing has to be involved for these events to be delivered.

Here's a concrete example that argues for supporting a non-routing-based way to learn of these server lifecycle events.

The code which manages metrics for virtual threads starts a Java Flight Recorder RecordingStream to receive virtual-thread related events from JFR. That code, which does not and should not rely on any routing information, has no way to learn when the server is about to/has shut down, so it cannot explicitly close the RecordingStream when the server is shutting down.

(Recall that meters can be retrieved programmatically from code in the server and not only by the metrics endpoint, so it's important to maintain these measurements apart from if they are ever retrieved via HTTP.)

Practically speaking that's fine because we need the RecordingStream as long as the server is up, and JVM shutdown is the ultimate clean-up.

But some recent work by @danielkec is somewhat disrupted by this - see #9687.

Arguably ServerLifecycle should be outside the webserver component and its implementations notified without routing...it's probably rare, but there could be Helidon servers that do not include a webserver but there are certainly components (like metrics) that should be able to respond to server lifecycle events without relying on routing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x OCA Verified All contributors have signed the Oracle Contributor Agreement. webserver
Projects
Status: Sprint Scope
Development

Successfully merging this pull request may close these issues.

3 participants