perwendel / spark

A simple expressive web framework for java. Spark has a kotlin DSL https://github.com/perwendel/spark-kotlin
Apache License 2.0
9.63k stars 1.56k forks source link

Initialization order wrong - causing 404 on Google App Engine java 11 Standard environment #1244

Open OndrejSpanel opened 2 years ago

OndrejSpanel commented 2 years ago

In Google App Engine Java 11 standard environment the whole application containing the embedded server is launched when a request is received which needs a new instance to be started. One notable example when this happens reproducibly each time after a new version of the application is deployed.

Spark starts the server whiles routes are defined, as addRoute contains a call to init. As a result the server starts listening as soon as you add the first route. This is a problems, because apparently as soon as the server is listening, GAE Java 11 can send a request to it. As a result it happens to me frequently while testing my application that the first request to obtain a web page after I deploy a new versions ends with various 404 error, as only the first route GET "/rest" is initialized, not other routes like GET "/app" or GET "/".

I think to be compatible with GAE on Java 11 standard environment it is necessary to be able to delay the server initialization until all routes are added.

nmondal commented 1 year ago

Spark runs in Java 8. I do have ( just ) created ( possibly fun ) version that runs on Java 17 with Jetty 11 jars, passing all integration tests. https://github.com/nmondal/spark-11

OndrejSpanel commented 1 year ago

@nmondal Being built on Java 8 is not a problem for Java 11. Problem for Java 11 GAE is a different way applications are started compared to GAE Java 8. The application needs to be able to serve any incoming request as soon as it starts listening with new GAE version. The Spark starts listening once you add the first route, because addRoute calls init. If a request which should be handled by another route is received before that route is added, meanwhile, it is rejected with 404 error. This happens frequently with new GAE as application is started on demand, as request are coming.

I am afraid your fork does not change this behaviour in any way.

nmondal commented 1 year ago

@OndrejSpanel given I am working on it, let's fix it then. Will it be possible for you to share a sample? Seems like the issue can be reproduced with external curl call and sleep between add routes?

tipsy commented 1 year ago

There is already another fork by @lepe: https://github.com/perwendel/spark/issues/1251#issuecomment-1177005112

This particular issue would probably best be solved by not automatically starting the server when a route is registered, but rather requiring a .start() call. Since the fork is also 2x, it would require something like Spark.requireManualStart which would need to be called first.

lepe commented 1 year ago

I think this could be solved by adding a field boolean autoStart = true inside Service, so you could do something like:

Service spark = Service.ignite();
spark.autoStart = false;
// add your routes here
spark.start();

Then, we would have to change inside Service:

// Keep backward compatibility (same meaning as it is now)
protected void init() { 
    start(true);
}
protected void start() {
    start(false);
}
// Previously `init()`
private void start(boolean auto) { 
    // add condition to initialize or not based on `auto` and `autoStart` values.
   // The rest of code as it is now in Service.init()
}

I could add it in my next release as I guess it won't have side effects.

@nmondal : I'm interested in your changes. I may check them and possibly add them to the next release. At the moment I would like to focus in updating Jetty's version so I can add http/3 support to Spark.

OndrejSpanel commented 1 year ago

Will it be possible for you to share a sample?

I have meanwhile converted my service to plain Jetty as it was not possible to use Spark in it because the issue.

The service is open source, the corresponding commit where Spark was removed is https://github.com/OndrejSpanel/Mixtio/commit/8556f1a82e1ed2af7f83871f84363d65dc15bfdc