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

Proposal for Early Connector Start #10664

Closed
wants to merge 2 commits into from

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Oct 4, 2023

This is proposal for offering a way to start connectors early for those deployments that want to have inter-context / inter-handler communications (via http requests) during the Server startup.

Such as having webappA deploy before webappB.
Then webappB having a load-on-start servlet init that makes an HTTP request to webappA to access something from it.

@joakime joakime added Enhancement Sponsored This issue affects a user with a commercial support agreement labels Oct 4, 2023
@joakime joakime added this to the 10.0.x milestone Oct 4, 2023
@joakime joakime requested review from gregw and sbordet October 4, 2023 15:41
@joakime joakime self-assigned this Oct 4, 2023
@@ -44,7 +44,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
{
Handler[] handlers = getHandlers();

if (handlers != null && isStarted())
Copy link
Contributor Author

@joakime joakime Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially controversial, as in order for the example in the PR to work, this isStarted() check cannot be here.

This seems arbitrary and not all Handler do this check.
Eg: ContextHandlerCollection and GzipHandler do not check isStarted(), BUT HandlerList, HandlerCollection, and RewriteHandler do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that checking is a bit arbitrary, but the check should be valid non-the-less

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is being looked at the wrong way. You don't want to start the connectors early, you want to deploy the webapps late (after everything else is started).

You could achieve this by not adding the deployer to the server until after the server is started. Or perhaps have an option of the app provider to not scan for apps during doStart and only scan after start.

@@ -44,7 +44,7 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
{
Handler[] handlers = getHandlers();

if (handlers != null && isStarted())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that checking is a bit arbitrary, but the check should be valid non-the-less

@gregw
Copy link
Contributor

gregw commented Oct 4, 2023

To give a bit more detail....

I think the best approach is to delay deploying the webapps, which could be done in one of several ways:

  1. Don't add the Deployer to the Server until after the Server is started. Essentially hot deploy the Deployer, to hot deploy the webapps.
  2. Add the Deployer, but modify it so that it does not do a scan for webapps during it's doStart. Use the normal hot deploy mechanism to subsequently deploy the webapps.
  3. Add the Deployer, but modify it so that it does not do a scan for webapps during it's doStart. Have a lifecycle listener added to the server that does the scan after the server is started.

All these could be done simply enough with embedded usage of Jetty. But we'd need to modify the Deployer if we want this behaviour from a java -jar start.jar start. We also need to add strong ordering of webapps discovered.

@joakime
Copy link
Contributor Author

joakime commented Oct 4, 2023

I have a different idea to try, different PR incoming.

@joakime
Copy link
Contributor Author

joakime commented Oct 4, 2023

@gregw how about PR #10667 approach?

@joakime
Copy link
Contributor Author

joakime commented Oct 4, 2023

Closing in favor of approach in PR #10667

@joakime joakime closed this Oct 4, 2023
@joakime joakime deleted the fix/10.0.x/early-connector-start branch October 25, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Sponsored This issue affects a user with a commercial support agreement
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants