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

Support multiprocess clients #40

Merged
merged 9 commits into from
Mar 25, 2024
Merged

Support multiprocess clients #40

merged 9 commits into from
Mar 25, 2024

Conversation

kegsay
Copy link
Member

@kegsay kegsay commented Mar 22, 2024

Fixes #26

This is quite a huge patch which does the following:

  • Define a new rpc binary in /cmd/rpc. This needs to be built BEFORE you run go test with the same -tags that are used in go test. This binary acts as an RPC server.
  • Add a new env var COMPLEMENT_CRYPTO_RPC_BINARY, which if set to the binary path, will enable RPC tests in internal/tests. If this is not set, RPC tests are ignored.
  • Add a new language binding deploy.RPCLanguageBindings which takes in just the RPC binary path and the language string you want to run. This binding has the following behaviour:
    • LanguageBindings.MustCreateClient: when called, the RPC server is spawned in a child process which listens on a random high numbered port, communicated via stdout. An RPC call is made with the client creation options and language to construct the client. This is why you need to build the RPC server with the same tags, as it makes a real client at this point, which needs FFI bindings / dist directory to be embedded for rust/js respectively. Returns a new type deploy.RPCClient which implements api.Client.
    • LanguageBindings.PreTestRun|PostTestRun: these do nothing in RPC mode. Logs are instead handled when the client is created/closed in the RPC server. A new contextID has been added to the interface in an attempt to allow logs to be split correctly (same file paths as today for the main process, and $user_$device scoped for the RPC process). TODO: logs don't always seem to come through?
  • The new api.Client for the most part just proxies the existing interface one-to-one. For example, MustBackpaginate does an RPC call with the same name, but bundles up the args/return param into a form that can be serialised by net/rpc. The exception is with the waiters:
    • WaitUntilEventInRoom returns a new type deploy.RPCWaiter which implements api.Waiter. Calling WaitUntilEventInRoom "registers" a waiter on the RPC server, which returns a unique waiter ID which is used in all calls which relate to that particular waiter.
    • Calling Waitf on this waiter does NOT just do a simple RPC call, but a complex dance to keep the API expressive. The problem lies in the function signature of WaitUntilEventInRoom which allows an arbitrary function to be the checker. We can't pass arbitrary functions over the RPC boundary. Instead:
      • calling Waitf will call the RPC function WaiterStart which calls TryWaitf server-side. The checker function used always returns false (so TryWaitf will always timeout with an error) but it also stashes the event that was asked to be checked.
      • the client will then periodically call the RPC function WaiterPoll every 100ms, which will then return the stashed events which can be serialised. When back in the client process, the arbitrary function can then be run against these events, and if one passes the check, great! If not, WaiterPoll is called again, until the RPC server decides to give up and time out the client.

@kegsay kegsay requested a review from andybalaam March 22, 2024 22:29
Copy link
Contributor

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Looks good, as far as I can see.

@andybalaam andybalaam merged commit 511da24 into main Mar 25, 2024
4 checks passed
@andybalaam andybalaam deleted the kegan/multiprocess branch March 25, 2024 13:14
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.

Multiprocess support
2 participants