fixGwhisperHangOnDescDbStreamClose #147
Conversation
abb3r
changed the title
…iption data. Signed-off-by: Rahman Abber Tahir <[email protected]>
Signed-off-by: Rahman Abber Tahir <[email protected]>
abb3r
force-pushed
the
fixGwhisperHangOnDescDbStreamClose
branch
from
afab6ce to
41810a5
Compare
Signed-off-by: Rahman Abber Tahir <[email protected]>
| @@ -350,12 +350,27 @@ void DescDbProxy::getDescriptors(const std::string &f_hostAddress) | |||
| } | |||
| } | |||
|
|
|||
| grpc::Status DescDbProxy::closeDescDbStream(std::optional<std::chrono::time_point<std::chrono::system_clock>> deadline) | |||
| { | |||
| if( m_disableCache == false )//cache enabled, no reflection stream required. | |||
There was a problem hiding this comment.
this is a slippery slope.
If cache is enabled, we will still use the reflection stream, if the cache entry is outdated or if the cache does not hold data for the host.
see line 330
There was a problem hiding this comment.
ah I see it. I change the condition to give prefence to m_reflectionDescDb check. if the ptr is valid we close the stream any how.
|
|
||
|
|
||
| //before calling the RPC, close the DescDb connection with a timeout. | ||
| grpc::Status dbDescStatus = ConnectionManager::getInstance().closeDescDbWithDeadline(serverAddress, deadline); |
There was a problem hiding this comment.
I was about to suggest moving all the closing into destructors and just drop the descriptor database / proxy when we do not need it any more.
However the ConnectionManager singleton makes life hard here. gWhisper does not have the most beautiful design :-(
So I am fine with it as is. what do you think?
There was a problem hiding this comment.
yeah ConnectionManager actually makes it hard. since we are also using shared ptrs here we would not know how many owners are there for the proxy and when will it actually closes.
Doing explicitly allows us to check the grpc status/result from calling the stream finish() aswell.
Yes we close the reflection stream after fecthing type/service info this we we don't couple the timeouts between reflection & main rpc. We also get the grpc status/result from the stream close by doing it early. |
…is invalid. Signed-off-by: Rahman Abber Tahir <[email protected]>
| if ( m_reflectionDescDb == nullptr ) | ||
| { | ||
| if( m_disableCache == false )//cache enabled, no reflection stream required. |
There was a problem hiding this comment.
What about just remove the check and return once m_reflectionDescDb == nullptr yields true? (meaning replace whe whole outer if body with a return;)
Sotty github woudn't let me write a suggestion for this.
There was a problem hiding this comment.
agreed, the unique ptr == null garauntees the stream is already closed. that is actually a valid case.
Signed-off-by: Rahman Abber Tahir <[email protected]>
close reflection stream with a deadline after fetching services description data.
Dynamic deadlines are now propagated to the DescDb stream close.
The db stream is now closed after 2 (after fetching name and type info from server).