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

feat: allow to stop the invoker #128

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ private static class FunctionClassLoader extends URLClassLoader {
private final String functionSignatureType;
private final ClassLoader functionClassLoader;

private Server server;

public Invoker(
Integer port,
String functionTarget,
Expand Down Expand Up @@ -225,8 +227,58 @@ ClassLoader getFunctionClassLoader() {
return functionClassLoader;
}

/**
* This will start the server and wait (join) for function calls.
* To start the server inside a unit or integration test, use {@link #startTestServer()} instead.
*
* @see #stopServer()
* @throws Exception
*/
public void startServer() throws Exception {
Server server = new Server(port);
startServer(true);
Copy link
Member

Choose a reason for hiding this comment

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

Could you put something like this first?

    if (server != null) {
      throw new IllegalStateException("Server already started");
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be better to do it inside the startServer(boolean) method instead of here ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you are right of course.

}


/**
* This will start the server and return.
*
* This method is designed to be used for unit or integration testing only.
* For other use cases use {@link #startServer()}.
*
* Inside a test a typical usage will be:
* <pre>
* {@code
* // Create an invoker
* Invoker invoker = new Invoker(
* 8081,
* "org.example.MyHttpFunction",
* "http",
* Thread.currentThread().getContextClassLoader()
* );
*
* // Start the test server
* invoker.startTestServer();
*
* // Test the function
*
* // Stop the test server
* invoker.stopServer();
* }
* </pre>
*
* @see #stopServer()
* @throws Exception
*/
public void startTestServer() throws Exception {
startServer(false);
}

private void startServer(boolean join) throws Exception {
if (server != null) {
throw new IllegalStateException("Server already started");
}

server = new Server(port);

ServletContextHandler servletContextHandler = new ServletContextHandler();
servletContextHandler.setContextPath("/");
Expand Down Expand Up @@ -261,7 +313,27 @@ public void startServer() throws Exception {

server.start();
logServerInfo();
server.join();
if (join) {
server.join();
}
}

/**
* Stop the server.
*
* @see #startServer()
* @see #startTestServer()
*
* @throws Exception
*/
public void stopServer() throws Exception {
if (server == null) {
throw new IllegalStateException("Server not yet started");
}

server.stop();
Copy link
Member

Choose a reason for hiding this comment

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

And here:

    if (server == null) {
     throw new IllegalStateException("Server not started");
   }

(which is a bit nicer than NullPointerException).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe setting the server to null after stopping it so the same invoker can be stoped and stated again without needed to create it again ?
This can be convenient if someone want to stop it / start it inside a @Before and create it inside an @BeforeAll ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, thanks for catching that, it should indeed be set to null after the stop() call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes done

// setting the server to null, so it can be started again
server = null;
}

private Class<?> loadFunctionClass() throws ClassNotFoundException {
Expand Down