-
Notifications
You must be signed in to change notification settings - Fork 9
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
fix!: handle socket name collisions #125
fix!: handle socket name collisions #125
Conversation
7e57788
to
0f651bd
Compare
I've added unit tests that work, as well as an integration tests that is not working. I will be debugging that tomorrow, probably by adding a bootstrap log to the backend process so I can actually debug what's going on. |
e2373d0
to
5c72013
Compare
Discussed with the team, we will be switching this to using environment variables instead, another revision coming |
connection_settings (ConnectionSettings, optional): The connection settings to use. | ||
This option is not required for the "init" command, but is required for everything | ||
else. Defaults to None. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a detail here mentioning that the connection_settings
are initialized in the init()
command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
"The file path to the connection file for use in daemon mode. For the 'daemon start' command, this file " | ||
"must not exist. For all other commands, this file must exist. This option is highly " | ||
"recommended if using the adaptor in an interactive terminal. Default is to read the " | ||
f"connection data from the environment variable: {_OPENJD_ADAPTOR_SOCKET_ENV}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you added some clarification on what the connection file is, but I think we could improve this a bit. How do you feel about something along the lines of
Used when running in daemon mode. Connection data is written to this filepath during the 'daemon start'
command and read in the 'daemon run/stop/cleanup' commands. If --connection-file is not provided then the
'{_OPENJD_ADAPTOR_SOCKET_ENV}' environment variable must be set with the connection file path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll add this
"recommended if using the adaptor in an interactive terminal. Default is to read the " | ||
f"connection data from the environment variable: {_OPENJD_ADAPTOR_SOCKET_ENV}" | ||
), | ||
"log_file": "The file to log adaptor output to. Default is to not log to a file.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"log_file": "The file to log adaptor output to. Default is to not log to a file.", | |
"bootstrap_log_file": "Only used in daemon _serve. An optional file path to log connection bootstrap output to.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we leave it as log_file
it seems like it should contain all adaptor logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can punt that to a future change. This change has already grown far too large in scope. Does that sound ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make this change in the future it then becomes another breaking change whereas making it before this is merged isn't breaking. In this case we're renaming an argument to make it (in my opinion) more clear what the purpose is so I'd prefer if we adopted it in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the parameter name to --bootstrap-log-file
@@ -64,7 +88,6 @@ | |||
os.path.join( | |||
_system_config_path_prefix, | |||
"openjd", | |||
"worker", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a breaking change right? Just want to make sure we call it out in the commit so that it is available in the changelog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I'll update the commit
@@ -123,13 +159,21 @@ class EntryPoint: | |||
The main entry point of the adaptor runtime. | |||
""" | |||
|
|||
on_bootstrap_complete: MutableSet[Callable[[], None]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that I like the list of callbacks pattern established here and above in the Background Runner. Right now we have a
- Set of callbacks in the
_LogConfig
which we add a single callback to in order to disconnect the file log handlers, but removes itself from the set of callbacks when called. - A set of callbacks in the BackgroundRunner which is only invoked when the connection file is written, which we added in order to provide the aforementioned callback
It looks like the goal is to disconnect the bootstrap logger after the connection file is written. I appreciate the forward thinking but I think in this case I'd prefer biasing towards keeping things simple and just having the background runner call a function directly which disconnects the bootstrap logging. It would allow us to keep the flow between these components more directly readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the idea is to make it generic so we do not need to refactor it in the future if we ever need to subscribe more actions to when the bootstrap is complete. Here I am trying to follow a really basic version of "events" in other languages, like C#.
Set of callbacks in the _LogConfig which we add a single callback to in order to disconnect the file log handlers, but removes itself from the set of callbacks when called.
This is an implementation detail of the event handler. It is designed to unsubscribe from the event after it's handled the first occurrence.
A set of callbacks in the BackgroundRunner which is only invoked when the connection file is written, which we added in order to provide the aforementioned callback
The fact that it happens after the connection file is written should be abstracted away from the caller. I named it on_bootstrap_complete
so that callers know that it happens after the BackendRunner is done bootstrapping, whatever that means (currently it means after the connection file is written, but may change in the future if we need to add more bootstrapping actions).
if not connection_file: | ||
raise RuntimeError( | ||
"--connection file is required for the '_serve' command but was not provided." | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we don't read from the environment variable here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point, I will add that ability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, thinking about this further. The environment variable OPENJD_ADAPTOR_SOCKET
points to the path for the socket that should be used for IPC, so that's different from the connection file. Currently, the BackendRunner is responsible for creating the socket and communicating that back to the frontend (via the --connection-file). We can change the backend to accept a path to try to create the socket into, however that currently does not happen.
Since the _serve
command is intended to be an "internal" command and is hidden from CLI help text, I think we can skip this for now. Does that sound ok?
dbc8a5d
to
6ff4dab
Compare
d28608b
to
4e7f93e
Compare
302f267
to
544873b
Compare
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
544873b
to
9f67f5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great stuff, Jericho. Thanks!
else: | ||
return True | ||
|
||
wait_for( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, nice. I actually just filed a bug for this. It looked like we've seen some data races on windows where the file can be opened, but hadn't yet contained data. Thanks for fixing!
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
Quality Gate passedIssues Measures |
What was the problem/requirement? (What/Why)
What was the solution? (How)
<pid>_<counter>
exists until it is available and use that.--log-file
option only exposed in the_serve
command) so that if the daemon bootstrap fails, we are able to look at the logs and determine what went wrong. Daemon bootstrap logging stops logging to the file after bootstrap is complete.--connection-file
CLI help textworker
path component from configuration file pathsOPENJD_ADAPTOR_SOCKET
.<adaptor> daemon start
command has been updated to emit anopenjd_env: OPENJD_ADAPTOR_SOCKET=<path-to-socket>
line to stdout so that, when running in an OpenJD environment, the option is automatically set and subsequent commands within this OpenJD environment no longer need to pass in a--connection-file
argument.--connection-file
argument is no longer necessary ifOPENJD_ADAPTOR_SOCKET
is provided<adaptor> daemon start
command will still generate a connection file at a temporary directory when it is not provided, then delete it after daemon bootstrap is complete.sockets.py
module to refer to "paths" rather than "directories"_utils._constants.py
fileWhat is the impact of this change?
<adaptor> daemon start
in an OpenJD environment no longer need to pass--connection-file
anymoreHow was this change tested?
Was this change documented?
No
Is this a breaking change?
No, the modified interface is internal to this package, under the
_http
packageManual testing
Test adaptor files
main.py
adaptor.py
Test logs - Linux with Env Var
Test Logs - Linux with connection file
Test Logs - Mac with env var
Test Logs - Mac with connection file
Test Logs - Windows with env var
Test Logs - Windows with connection file
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.