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

Timeouts for CS, SS, DS should mean the time to the initial message response from the server side #52

Closed
3 tasks done
amydevs opened this issue Nov 1, 2023 · 7 comments · Fixed by #53
Closed
3 tasks done
Assignees
Labels
development Standard development r&d:polykey:supporting activity Supporting core activity

Comments

@amydevs
Copy link
Contributor

amydevs commented Nov 1, 2023

Specification

Timeouts for Streaming Calls should only be until the initial message. Once the timeout has been fulfilled, there should be no timeouts that affect the call after that point.

Client

For the client, the timeout is the time that it will wait until getting the first message from the peer.

Server

The timeout will stay advisory for servers, so the server can throw optionally as they need. Timeouts are now understood as the time the client will wait for until the first message.

Additional context

Tasks

  1. Apply timeout changes to RPCClient
  2. Apply timeout changes to RPCServer
  3. Rewrite tests for new timeouts
@amydevs amydevs added the development Standard development label Nov 1, 2023
@CMCDragonkai
Copy link
Member

So the README.md was updated, but actually implementation wasn't done.

The point is that timeouts for RPC when it comes to CS, SS, DS shouldn't be per-message timeouts. And they shouldn't be per-call timeouts.

The only timeout that makes sense here, is timeout to FIRST response.

THUS:

  1. RPCClient timeout means that for unary, CS, SS, DS, means it is the time I will wait for until I get the first message from the other side whatever that is. For unary this behaviour is the same as is, for the others it allows long-running RPC streams.
  2. RPCServer timeout is advisory, thus handlers can decide what to do here. But generally speaking it should be understood as the time the client will wait for until the first message. For unary this is the same meaning. For the others, if the timeout is exhausted, and I haven't sent the initial message response, then it should throw. But if it is the case I have already responded, then I don't need to worry about the timer anymore. This has implications on the implementation of the SS, CS, DS handlers, since they shouldn't be passing the ctx down to any calls UNLESS it's part of the process before the initial response message. Otherwise, handlers may still maintain its own timers when executing things it expects to take some time to yield a result. YMMV.

@amydevs I need the tasks fleshed out here.

Will this impact the #27?

@CMCDragonkai
Copy link
Member

In the future @amydevs do not write generic issue titles. Issues must be specific!

@CMCDragonkai CMCDragonkai changed the title Revised TImeouts for Streaming Calls Timeouts for CS, SS, DS should mean the time to the initial message response from the server side Nov 1, 2023
@CMCDragonkai
Copy link
Member

In relation to Node Connections, we would say that as long as the RPC stream is live, then NCs should not expire. Because it could mean there's a long running RPC call happening, and this is true even if there is no RPC activity (no messages). At the bottom in the transport layer, there can still be keep alive packets, these are not the same thing.

@tegefaulkes can you confirm this?

@tegefaulkes
Copy link
Contributor

The TTL timer for the NodeConnections in the connection map is disable when the connection is being used to make a call. That's if there is a withConnF being used.

I also added this logic to the reverse streams in #609. So yeah, if the connection is actively being used then it will not time out from the TTL.

@amydevs
Copy link
Contributor Author

amydevs commented Nov 2, 2023

So the README.md was updated, but actually implementation wasn't done.

The point is that timeouts for RPC when it comes to CS, SS, DS shouldn't be per-message timeouts. And they shouldn't be per-call timeouts.

The only timeout that makes sense here, is timeout to FIRST response.

THUS:

1. RPCClient timeout means that for unary, CS, SS, DS, means it is the time I will wait for until I get the first message from the other side whatever that is. For unary this behaviour is the same as is, for the others it allows long-running RPC streams.

2. RPCServer timeout is advisory, thus handlers can decide what to do here. But generally speaking it should be understood as the time the client will wait for until the first message. For unary this is the same meaning. For the others, if the timeout is exhausted, and I haven't sent the initial message response, then it should throw. But if it is the case I have already responded, then I don't need to worry about the timer anymore. This has implications on the implementation of the SS, CS, DS handlers, since they shouldn't be passing the ctx down to any calls UNLESS it's part of the process before the initial response message. Otherwise, handlers may still maintain its own timers when executing things it expects to take some time to yield a result. YMMV.

@amydevs I need the tasks fleshed out here.

Will this impact the #27?

I don't think should impact #27

@CMCDragonkai
Copy link
Member

The TTL timer for the NodeConnections in the connection map is disable when the connection is being used to make a call. That's if there is a withConnF being used.

I also added this logic to the reverse streams in #609. So yeah, if the connection is actively being used then it will not time out from the TTL.

And by actively used you mean it will support RPC streams that have no activity yet but the stream is still alive.

@tegefaulkes
Copy link
Contributor

Yes.

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:supporting activity Supporting core activity
Development

Successfully merging a pull request may close this issue.

3 participants