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

Conversation

loicmathieu
Copy link
Contributor

@loicmathieu loicmathieu commented Feb 14, 2022

Allow to stop the invoker so it can be easily used inside an integration test like this.

Fixes #18

        // start the invoker without joining to avoid blocking the thread
        Invoker invoker = new Invoker(
                8081,
                "org.example.HelloWorldHttpFunction",
                "http",
                Thread.currentThread().getContextClassLoader()
        );
        invoker.startServer(false);

        // test the function using RestAssured
        when()
                .get("http://localhost:8081")
                .then()
                .statusCode(200)
                .body(is("Hello World!"));

        // stop the invoker
        invoker.stopServer();

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I think it will be very useful for people testing their functions. I did have a few requests.

@loicmathieu loicmathieu force-pushed the stop-the-invoker branch 2 times, most recently from b2a1b0a to de3ba8d Compare February 18, 2022 10:42
@eamonnmcmanus
Copy link
Member

That looks great, thanks! Do you know why the indentation of some lines has changed? The changes don't look like improvements.

@loicmathieu
Copy link
Contributor Author

@eamonnmcmanus sorry, something may happens when I try to fix the format issue using the Google formatter tool.

I revert the identation changes.

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

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

The google-java-format check is failing because of these diffs:

8:25:13.000000000 -0800
+++ -	2022-02-21 08:27:05.000000000 -0800
@@ -228,8 +228,8 @@
   }

   /**
-   * 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.
+   * 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
@@ -238,31 +238,21 @@
     startServer(true);
   }

-
   /**
    * 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()}.
+   * <p>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:
-   * {@code
-   *         // Create an invoker
-   *         Invoker invoker = new Invoker(
-   *                 8081,
-   *                 "org.example.MyHttpFunction",
-   *                 "http",
-   *                 Thread.currentThread().getContextClassLoader()
-   *         );
+   * <p>Inside a test a typical usage will be: {@code // Create an invoker Invoker invoker = new
+   * Invoker( 8081, "org.example.MyHttpFunction", "http",
+   * Thread.currentThread().getContextClassLoader() );
    *
-   *         // Start the test server
-   *         invoker.startTestServer();
+   * <p>// Start the test server invoker.startTestServer();
    *
-   *         // Test the function
+   * <p>// Test the function
    *
-   *         // Stop the test server
-   *         invoker.stopServer();
-   * }
+   * <p>// Stop the test server invoker.stopServer(); }
    *
    * @see #stopServer()
    * @throws Exception
@@ -317,7 +307,6 @@
    *
    * @see #startServer()
    * @see #startTestServer()
-   *
    * @throws Exception
    */
   public void stopServer() throws Exception {

I am not sure why you are seeing the indentation change when you run g-j-f. Perhaps --aosp is set? Anyway the main thing is that you need <pre>...</pre> around the {@code...} block.

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.

* @throws Exception
*/
public void stopServer() throws Exception {
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

@eamonnmcmanus
Copy link
Member

eamonnmcmanus commented Feb 21, 2022

Thanks again! I'll fix the formatting problems myself, since the formatter works fine for me.

@eamonnmcmanus eamonnmcmanus merged commit 14908ca into GoogleCloudPlatform:master Feb 21, 2022
@loicmathieu
Copy link
Contributor Author

Thanks @eamonnmcmanus to have run the formater for me.

@loicmathieu loicmathieu deleted the stop-the-invoker branch February 22, 2022 09:28
anniefu pushed a commit to anniefu/functions-framework-java that referenced this pull request Sep 7, 2022
🤖 I have created a release *beep* *boop*
---


## 1.0.0 (2022-09-07)


### Features

* allow to stop the invoker
([GoogleCloudPlatform#128](https://github.com/anniefu/functions-framework-java/issues/128))
([14908ca](14908ca))
* enable converting CloudEvent requests to Background Event requests
([GoogleCloudPlatform#123](https://github.com/anniefu/functions-framework-java/issues/123))
([1c4a014](1c4a014))
* Increase maximum concurrent requests for jetty server to 1000.
([GoogleCloudPlatform#144](https://github.com/anniefu/functions-framework-java/issues/144))
([439d0b5](439d0b5))


### Bug Fixes

* Add build env vars support for function deployment.
([GoogleCloudPlatform#133](https://github.com/anniefu/functions-framework-java/issues/133))
([0e052f3](0e052f3))
* bump dependency versions
([GoogleCloudPlatform#134](https://github.com/anniefu/functions-framework-java/issues/134))
([faff79d](faff79d))
* make user function exceptions log level SEVERE
([GoogleCloudPlatform#113](https://github.com/anniefu/functions-framework-java/issues/113))
([1684c0e](1684c0e))
* update conformance tests
([GoogleCloudPlatform#108](https://github.com/anniefu/functions-framework-java/issues/108))
([72852d0](72852d0))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invoker.stopServer method is missing
2 participants