[Draft] Add GraphQL Subscription to ControllerClient #163
Conversation
zhengfeihe
force-pushed
the
addSubscriptionClient
branch
from
ca53819
to
ef1c4dd
Compare
| rRequestStringBuffer.Clear(); | ||
| } | ||
|
|
||
| std::string ControllerClientImpl::SpinOnce(){ |
There was a problem hiding this comment.
Not sure whether we should process the read message on the client side:
- Parse the message with RapidJSON .
- Skip the keep-alive message {"type":"ka"} and continue reading until we receive a valid message.
There was a problem hiding this comment.
If it is the user's job to call SpinOce, then you don't need the SpinOnce function. SpinOnce is usually used when there is a background thread. In this case, maybe you can consider to have a function GetSubriptionResult or something similar to let the use to fetch the data when they need it.
| /// \brief Execute GraphQL subscription query against Mujin Controller. | ||
| /// | ||
| /// Throws an exception if there are any errors | ||
| virtual void ExecuteGraphSubscription(const char* operationName, const char* query, const rapidjson::Value& rVariables, rapidjson::Document::AllocatorType& rAlloc) = 0; |
There was a problem hiding this comment.
Follow the ExecuteGraphQuery
There was a problem hiding this comment.
It's okay, but better to use mordern C++.
| @@ -475,6 +484,38 @@ void ControllerClientImpl::ExecuteGraphQueryRaw(const char* operationName, const | |||
| _ExecuteGraphQuery(operationName, query, rVariables, rResult, rAlloc, timeout, false, true); | |||
| } | |||
|
|
|||
| void ControllerClientImpl::ExecuteGraphSubscription(const char* operationName, const char* query, const rapidjson::Value& rVariables, rapidjson::Document::AllocatorType& rAlloc) | |||
There was a problem hiding this comment.
why don't you have a rapidjson::MemoryPoolAllocator<> in the member, so you don't need to keep passing rAlloc?
There was a problem hiding this comment.
I was thinking of doing this, but I followed the current deign of ExecuteGraphQuery for consistency, otherwise we need to change both
There was a problem hiding this comment.
Subscription is a frequent job and will lead to some overhead. Better to use the memory pool.
| rPayLoad.AddMember(rapidjson::Document::StringRefType("variables"), rValue, rAlloc); | ||
| rValue.SetString("start", rAlloc); | ||
| rRequest.AddMember(rapidjson::Document::StringRefType("type"), rValue, rAlloc); | ||
| rRequest.AddMember(rapidjson::Document::StringRefType("payload"), rPayLoad, rAlloc); |
There was a problem hiding this comment.
it's hard to read. Add some empty line between L496-507.
|
|
||
| rapidjson::Writer<rapidjson::StringBuffer> writer(rRequestStringBuffer); | ||
| rRequest.Accept(writer); | ||
| _pWebsocketStream->write(rRequestStringBuffer.GetString()); |
There was a problem hiding this comment.
Check other client implementations to see how to efficiently reuse the buffer.
There was a problem hiding this comment.
Still, follow the ExecuteGraphQuery for consistency
| void ControllerClientImpl::CreateWebSocketStream() | ||
| { | ||
| boost::asio::io_context io_context; | ||
| MUJIN_LOG_INFO(_clientInfo.host); |
There was a problem hiding this comment.
This is just for test. I will remove it
| socket.connect(endpoint); | ||
| _pWebsocketStream = std::make_unique<UnixWebSocketStream>(std::move(socket)); | ||
| } | ||
| std::string usernamepassword = _clientInfo.username + ":" + _clientInfo.password; |
There was a problem hiding this comment.
Using boost::format might have slightly better performance.
| std::string usernamepassword = _clientInfo.username + ":" + _clientInfo.password; | ||
| std::string encodedusernamepassword = GetEncodeBase64String(usernamepassword); | ||
| _pWebsocketStream->set_option(usernamepassword, encodedusernamepassword); | ||
| _pWebsocketStream->handshake(_clientInfo.host, "/api/v2/graphql"); |
There was a problem hiding this comment.
Extract the endpoint as a static contexpr std::string.
| return message; | ||
| } | ||
|
|
||
| void TCPWebSocketStream::close() { |
There was a problem hiding this comment.
Maybe also protected by boost::mutex::scoped_lock lock(_mutex);?
| rRequestStringBuffer.Clear(); | ||
| } | ||
|
|
||
| std::string ControllerClientImpl::SpinOnce(){ |
There was a problem hiding this comment.
If it is the user's job to call SpinOce, then you don't need the SpinOnce function. SpinOnce is usually used when there is a background thread. In this case, maybe you can consider to have a function GetSubriptionResult or something similar to let the use to fetch the data when they need it.
Enable GraphQL subscription based on WebSocket