feat: Implement the background mode in Windows. #23

Honglichenn · 2023-11-14T22:57:58Z

What was the problem/requirement? (What/Why)

Background mode is missing in the Windows, we need to implement it.

What was the solution? (How)

Use Named Pipe for IPC solution in Windows to implement the background mode.
There are lots of TODO in this PR. Those missing function will be added later to the package. I just want to limit the changes in a single PR for easily reviewed.

What is the impact of this change?

Background mode can be run in Windows.

How was this change tested?

Enable the related integration test and all of them are passed.

Was this change documented?

Yes

Is this a breaking change?

No. This change should not affect any existing Linux implementation.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

src/openjd/adaptor_runtime/_background/backend_named_pipe_server.py

src/openjd/adaptor_runtime/_background/named_pipe_request_handler.py

src/openjd/adaptor_runtime/_background/server_config.py

src/openjd/adaptor_runtime/adaptors/configuration/_configuration_manager.py

src/openjd/adaptor_runtime/application_ipc/__init__.py

src/openjd/adaptor_runtime/_background/backend_named_pipe_server.py

src/openjd/adaptor_runtime/_background/backend_runner.py

AWS-Samuel · 2023-11-20T17:03:09Z

src/openjd/adaptor_runtime/_background/backend_runner.py

-
-            # Cleanup the connection file and socket
-            for path in [self._connection_file_path, socket_path]:
+            self._server.shutdown()  # type: ignore


I see, we could add a if self._server is not None: here, I think we generally want to avoid using type: ignore whenever possible so that our type checking is stronger.

src/openjd/adaptor_runtime/_background/named_pipe_request_handler.py

src/openjd/adaptor_runtime/_background/frontend_runner.py

src/openjd/adaptor_runtime/_background/named_pipe_request_handler.py

src/openjd/adaptor_runtime/application_ipc/__init__.py

src/openjd/adaptor_runtime/adaptors/configuration/_configuration_manager.py

src/openjd/adaptor_runtime/_background/backend_runner.py

src/openjd/adaptor_runtime/_background/frontend_runner.py

ddneilson · 2023-11-20T16:23:21Z

src/openjd/adaptor_runtime/_background/backend_named_pipe_server.py

+        _logger.info(f"Creating Named Pipe with name: {self._pipe_name}")
+        # During shutdown, a `True` will be pushed to the `_cancel_queue` for ending this loop
+        while self._cancel_queue.qsize() == 0:
+            pipe_handle = self._create_pipe(self._pipe_name)


Do we need to be creating a new pipe file for every request, or is there a way to reuse the same one? It seems weird to me to only be able to send a single request per pipe.

Also, you're not handling the pipe_handle == None case here.

For the reusing the NamedPipe instance, I will try to explain this. Short answer is there are not simple ways to do that. If we reuse the named pipe instance, this situation may happen, a run command is sent to the server followed by a stop command, the response for the run command may be read by the stop command. We are creating the NamedPipe instance here under the same NamedPipe server. The relationship between a NamedPipe server and a NamedPipe instance can be likened to the relationship between an HTTP server and an HTTP connection . And in the HTTP/2 connection, they have a technique called multiplexing (multiple requests and responses can be sent concurrently over a single connection). NamedPipe doesn't support this (like HTTP/1).. we can implement this by ourselves if we observe performance issues.

Good catch, I will handle it in next version.

src/openjd/adaptor_runtime/_background/backend_named_pipe_server.py

test/openjd/adaptor_runtime/integ/background/test_background_mode.py

src/openjd/adaptor_runtime/_background/server_config.py

jericht · 2023-11-22T15:35:13Z

src/openjd/adaptor_runtime/_background/frontend_runner.py

+        if OSName.is_windows():
+            return self._send_windows_request(
+                method,
+                path,
+                params=params if params else None,
+                json_body=json_body if json_body else None,
+            )
+        else:
+            return self._send_linux_request(
+                method,
+                path,
+                params=params if params else None,
+                json_body=json_body if json_body else None,
+            )


Instead of this pattern, what do you think about specializing this class and having two implementations with an abstract method for _send_request? e.g. we can have WindowsFrontendRunner and PosixFrontendRunner that inherit this base class and only have a _send_request method (for now)

Then we can move the if windows elif posix stuff to outside of this class wherever we instantiate the FrontendRunner.

Yes, similar to above discussion, will do it outside of this PR.

jericht · 2023-11-22T17:28:01Z

src/openjd/adaptor_runtime/_background/named_pipe_request_handler.py

+            _logger.error(
+                f"Encountered an error while closing the named pipe: {traceback.format_exc()}"
+            )
+        _logger.debug("WinBackgroundResourceRequestHandler instance thread exited.")


I think we should bump this up to INFO level so it's clear in the logs when this thread exits

This will create lots of unnecessary log because we will create the thread when establishing the connection. I don't think it is necessary to log the information when the instance thread existed. Also we are running the server in multi-thread environment so logging this won't help for debugging because we don't know which thread existed.

src/openjd/adaptor_runtime/_background/named_pipe_helper.py

src/openjd/adaptor_runtime/_background/backend_named_pipe_server.py

Signed-off-by: Hongli Chen <[email protected]>

Daniel is out this week and I already addressed his comments and for the optimization part, I will do it outside of this PR.

Honglichenn force-pushed the honglich/win_backend branch 30 times, most recently from 8510a74 to 209eb6f Compare November 16, 2023 22:25

Honglichenn requested review from ddneilson and AWS-Samuel November 16, 2023 23:14

Honglichenn marked this pull request as ready for review November 16, 2023 23:14

Honglichenn requested a review from a team as a code owner November 16, 2023 23:14

AWS-Samuel requested changes Nov 17, 2023

View reviewed changes

Honglichenn force-pushed the honglich/win_backend branch 2 times, most recently from e51dcab to c76753b Compare November 17, 2023 21:33

Honglichenn requested a review from AWS-Samuel November 17, 2023 22:06

AWS-Samuel requested changes Nov 20, 2023

View reviewed changes

Honglichenn force-pushed the honglich/win_backend branch from c76753b to 6b1eb07 Compare November 20, 2023 17:41

Honglichenn requested a review from AWS-Samuel November 20, 2023 17:41

ddneilson previously requested changes Nov 20, 2023

View reviewed changes

AWS-Samuel previously approved these changes Nov 20, 2023

View reviewed changes

Honglichenn dismissed AWS-Samuel’s stale review via ffcc3b6 November 20, 2023 19:43

Honglichenn force-pushed the honglich/win_backend branch from 6b1eb07 to ffcc3b6 Compare November 20, 2023 19:43

Honglichenn requested a review from ddneilson November 20, 2023 19:53

Honglichenn force-pushed the honglich/win_backend branch from ffcc3b6 to 8fd5134 Compare November 20, 2023 20:11

Honglichenn requested a review from AWS-Samuel November 21, 2023 16:21

jericht self-requested a review November 21, 2023 17:19

jericht requested changes Nov 22, 2023

View reviewed changes

feat: Implement the background mode in Windows.

150270a

Signed-off-by: Hongli Chen <[email protected]>

Honglichenn force-pushed the honglich/win_backend branch from 8fd5134 to 150270a Compare November 27, 2023 21:16

Merge branch 'mainline' into honglich/win_backend

ce19774

Honglichenn requested a review from jericht November 28, 2023 17:30

jericht approved these changes Nov 28, 2023

View reviewed changes

AWS-Samuel approved these changes Nov 28, 2023

View reviewed changes

Honglichenn merged commit ac80ce7 into mainline Nov 28, 2023
9 checks passed

Honglichenn deleted the honglich/win_backend branch November 28, 2023 19:13

ddneilson added the security Pull requests that could impact security label Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement the background mode in Windows. #23

feat: Implement the background mode in Windows. #23

Honglichenn commented Nov 14, 2023 •

edited

Loading

AWS-Samuel Nov 20, 2023

ddneilson Nov 20, 2023

Honglichenn Nov 20, 2023

jericht Nov 22, 2023

Honglichenn Nov 27, 2023

jericht Nov 22, 2023

Honglichenn Nov 27, 2023

feat: Implement the background mode in Windows. #23

feat: Implement the background mode in Windows. #23

Conversation

Honglichenn commented Nov 14, 2023 • edited Loading

What was the problem/requirement? (What/Why)

What was the solution? (How)

What is the impact of this change?

How was this change tested?

Was this change documented?

Is this a breaking change?

AWS-Samuel Nov 20, 2023

Choose a reason for hiding this comment

ddneilson Nov 20, 2023

Choose a reason for hiding this comment

Honglichenn Nov 20, 2023

Choose a reason for hiding this comment

jericht Nov 22, 2023

Choose a reason for hiding this comment

Honglichenn Nov 27, 2023

Choose a reason for hiding this comment

jericht Nov 22, 2023

Choose a reason for hiding this comment

Honglichenn Nov 27, 2023

Choose a reason for hiding this comment

Honglichenn commented Nov 14, 2023 •

edited

Loading