Watchdog with automatic inbound flow control should start with WAITING state #2498
Comments
4 tasks
lqiu96
added a commit
that referenced
this issue
Feb 23, 2024
Watchdog should start with WAITING state, and only switch to `idle` if auto flow control was disabled. Before the fix, when auto flow control was enabled, we wait for server to return a response without calling `onRequest()` and watchdog would report the timeout exception because of idle timeout, which is incorrect and causes confusion. Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/gapic-generator-java/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) Fixes #2498 ☕️ --------- Co-authored-by: Igor Bernstein <igorbernstein@google.com> Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>
lqiu96
added a commit
that referenced
this issue
Feb 26, 2024
Watchdog should start with WAITING state, and only switch to `idle` if auto flow control was disabled. Before the fix, when auto flow control was enabled, we wait for server to return a response without calling `onRequest()` and watchdog would report the timeout exception because of idle timeout, which is incorrect and causes confusion. Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/gapic-generator-java/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) Fixes #2498 ☕️ --------- Co-authored-by: Igor Bernstein <igorbernstein@google.com> Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>
lqiu96
added a commit
that referenced
this issue
Feb 28, 2024
Watchdog should start with WAITING state, and only switch to `idle` if auto flow control was disabled. Before the fix, when auto flow control was enabled, we wait for server to return a response without calling `onRequest()` and watchdog would report the timeout exception because of idle timeout, which is incorrect and causes confusion. Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/gapic-generator-java/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) Fixes #2498 ☕️ --------- Co-authored-by: Igor Bernstein <igorbernstein@google.com> Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
There are 2 states in watchdog, IDLE and WAITING. IDLE indicates the stream is waiting for caller to take some action, and WAITING indicates the stream is waiting for server to return something. We update the state whenever there's some activity from the server or caller, and record timestamps when the state changes. When a stream is inactive for too long, watchdog will cancel the stream and throw an exception.
In the current implementation when we create a
WatchdogStream, we always set the stream state to "IDLE". The "IDLE" state is flipped to "WAITING" state when caller callsrequest(). However, this implementation is assuming that automatic inbound flow control is disabled. In the case where automatic inbound flow control is enabled, caller won't callrequest()for the next response and in this case starting theWatchdogStreamin the IDLE state is incorrect.The text was updated successfully, but these errors were encountered: