add hard-coded deadline for reflection stream. #150
Conversation
Signed-off-by: Rahman Abber Tahir <[email protected]>
Signed-off-by: Rahman Abber Tahir <[email protected]>
…gracefully. Signed-off-by: Rahman Abber Tahir <[email protected]>
abb3r
changed the title
Signed-off-by: Rahman Abber Tahir <[email protected]>
Signed-off-by: Rahman Abber Tahir <[email protected]>
| @@ -32,6 +32,7 @@ using grpc::reflection::v1alpha::ServerReflection; | |||
| using grpc::reflection::v1alpha::ServerReflectionRequest; | |||
| using grpc::reflection::v1alpha::ServerReflectionResponse; | |||
|
|
|||
| const uint8_t g_timeoutGrpcMainStreamSeconds = 20; //using default gwhisper timeout of 20 seconds. | |||
There was a problem hiding this comment.
increased since we already have default rpc timeout of 30secs anyway, is that too big? we saw that 10secs easily times out on slower systems.
|
Sorry, did not have time to review this week, will have a look, as soon as I have a free minute next week :-) |
src/libCli/Call.cpp
Outdated
| } | ||
| //before calling the RPC, close the DescDb connection with a default timeout. We still continue with rpc call | ||
| //but remove the cache file if the stream was not closed gracefully so it fetches the reflection data again next time. | ||
| grpc::Status dbDescStatus = ConnectionManager::getInstance().closeDescDbStream(serverAddress); |
There was a problem hiding this comment.
| grpc::Status dbDescStatus = ConnectionManager::getInstance().closeDescDbStream(serverAddress); | |
| ConnectionManager::getInstance().closeDescDbStream(serverAddress); |
src/libCli/ConnectionManager.cpp
Outdated
| @@ -75,7 +74,7 @@ namespace cli | |||
| } | |||
|
|
|||
| //if proxy exists close the stream with a deadline. | |||
There was a problem hiding this comment.
| //if proxy exists close the stream with a deadline. | |
| //if proxy exists close the stream |
| status = m_reflectionDescDb->closeDescDbStream(); | ||
| if(not status.ok()) | ||
| { | ||
| //failure to close stream leads to invalid cache, |
| DescDbProxy::~DescDbProxy() | ||
| { | ||
| closeDescDbStream(); //close it here to ensure invalid cache file is removed if an error occurs. |
There was a problem hiding this comment.
well, isn't destructor called each time gwhisper terminates?
If so, we would invalidate the cache each time. This would make the cache use-less :P It's only purpose is to transport descriptr info from one invocation the the next.
(e.g. every time a user presses gwhisper is started afresh and parses the input / gets descriptors without cache)
There was a problem hiding this comment.
yes the proxy dtor is called evrytime.
actually the reflection stream if already closed gracefully will be make the close at proxy dtor a noop since we check for nullptr.
The cache is only invalidated if we fail to close the stream in time. so we don't disable it and the functional tests are green to prove it.
There was a problem hiding this comment.
Ok, understand. Maybe add a comment in the destructor.
Then it should be OK
Signed-off-by: Rahman Abber Tahir <[email protected]>
Signed-off-by: Rahman Abber Tahir <[email protected]>
Signed-off-by: Rahman Abber Tahir <[email protected]>
No description provided.