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

Update GRPC client calls and service handlers to support cancellations and deadlines #466

Closed
tegefaulkes opened this issue Sep 29, 2022 · 9 comments
Assignees
Labels
development Standard development r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management r&d:polykey:supporting activity Supporting core activity

Comments

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Sep 29, 2022

Specification

GRPC calls need to support cancellation and deadlines. This means that the GRPC client call needs to respond to a abortion and stop the current call. The GRPC service handler will also need to respond to an abortion, either triggered by a deadline connection failure or the client abortion, and finish processing the request early.

Additional context

Tasks

  1. Determine how to interface with the GRPC API to implement timeouts and cancellablility.
  2. Convert the Client side methods to use timedCancellable.
  3. Covert the service handlers to use timeouts and cancellability
  4. Update and use the test client/ handlers to do the same and make tests to check the behaviour.
@tegefaulkes tegefaulkes added the development Standard development label Sep 29, 2022
@tegefaulkes tegefaulkes self-assigned this Sep 29, 2022
@tegefaulkes
Copy link
Contributor Author

This one is pretty complex. We need to apply cancellation to the GRPC client calls, this includes the timeouts using timedCancellable. We also need to implement this for the service handlers where we set up a timer based on the metadata and abortion if the connection ends.

The first stage will deal with researching the GRPC API to work out how to hook up cancel ability with the API. This includes aborting the client connection, setting a timeout for the client calls, getting the timeouts on the service side and handling connection drops. The next stage is updating the client side to make use of these and then converting all of the service handlers.

Needs some tests for good measure, but we don't want to test EVERY client call or service handler. I'll have to look into it a little but I think we can make use of the test client/server for this? Might be a good place to start.

@CMCDragonkai
Copy link
Member

Yea I think it's enough to test that the mechanism works for unary, client streaming, server streaming and bidi streaming and that's it.

@CMCDragonkai
Copy link
Member

The most important RPC calls will be the ones that have "long running things" that depend on other agents like node cross claim.

@CMCDragonkai
Copy link
Member

Hey @tegefaulkes instead of just setting it to 8, anything larger than a 5 should be broken down into smaller issues so that way it's easier to track separately. I think this issue in particular may require an epic to split out in to all the domains.

@CMCDragonkai
Copy link
Member

Also once it works on one domain, I'm sure it can be copied out to all the other ones.

@CMCDragonkai
Copy link
Member

In terms of reserving the meta property of the request params:

{
  params: {
    meta: {
      token: "abc"
      deadline: 342341,
    }
  }
}

This should allow the client to communicate to the agent how much time it wants to give the agent to complete the work.

Even if the client doesn't send a deadline or timeout, the agent still needs to have a default timeout for each handler.

Furthermore there should be a limit to the deadline - how large it can be.

There are some situations where a deadline doesn't apply. It is possible in some cases for the deadline to just be null. Notice that null is separate from undefined. Where null is an explicit configuration of having no deadline at all.

This means we can have push-flows that last forever. But still rely on the underlying transport idleness timer that can close connections if nothing progresses there.

@CMCDragonkai
Copy link
Member

Ideas regarding request metadata:

  1. token
  2. deadline
  3. version

Ideas regarding response metadata:

  1. token
  2. version

@tegefaulkes
Copy link
Contributor Author

Yep, we can add these fields the the request and response metadata wrapper types. We can use the middleware to enforce the version requirements. The deadlines needs a little bit of thought since we need to pass it into the handler using the provided context object. So either deadline support needs to be hard coded into the RPC OR the middleware needs access to an abort controller so it can abort the current handler on a condition such as a timer.

@CMCDragonkai
Copy link
Member

This is already done in #510. The JSON RPC system supports this now. We no longer are going to be using GRPC. It's just a matter of doing agent migration to introduce the deadlines.

@CMCDragonkai CMCDragonkai added r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management r&d:polykey:supporting activity Supporting core activity labels Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management r&d:polykey:supporting activity Supporting core activity
Development

No branches or pull requests

2 participants