jsonrpc: Wait for the socket to be ready to read. #11815
Conversation
We found out that sometimes we hit a busy loop here, so adding a timeout to make sure we do not hangup forever here.
brbzull0
changed the title
There was a problem hiding this comment.
I am not sure this is correct. Should you call read_ready if there were some bytes read in previous iterations of the outer loop? Shouldn't a timeout from read_ready stop the inner loop? It seems like this will always wait until EOF or until (timeout_ms * attempts) have elapsed. The logic is a bit confusing, so maybe some comments about what is happening will clear it up.
| @@ -30,12 +30,12 @@ | |||
| #include "shared/rpc/MessageStorage.h" | |||
| #include <tscore/ink_assert.h> | |||
| #include <tscore/ink_sock.h> | |||
|
|
|||
| #include <iostream> | |||
| @@ -117,23 +117,36 @@ IPCSocketClient ::send(std::string_view data) | |||
| } | |||
|
|
|||
| IPCSocketClient::ReadStatus | |||
| IPCSocketClient::read_all(std::string &content) | |||
| IPCSocketClient::read_all(std::string &content, std::chrono::milliseconds timeout_ms, int attempts) | |||
| { | |||
| if (this->is_closed()) { | |||
| // we had a failure. | |||
| return {}; | |||
There was a problem hiding this comment.
I know it's not part of the changes, but this will return NO_ERROR? But the comment indicates a failure. Could UNKNOWN be a better return?
| // done! | ||
| break; | ||
| } | ||
| } while (ret != 0 && attempts_left > 0); |
There was a problem hiding this comment.
Should this check for readStatus timeout and error and also stop the loop? If you don't check for timeout, this function will wait for timeout_ms * attempts (10 seconds by default). What errors would poll return that we would want to retry?
We found out that sometimes we hit a busy loop here, so adding a timeout to make sure we do not hangup forever here.