C++ client low level API #8420

BrennanConroy · 2019-03-11T21:49:29Z

Please look over the API and ignore tests.

http_client: https://github.com/aspnet/AspNetCore/blob/28ccc03fb90499425a9a3deb0c066e8624fff23a/src/SignalR/clients/cpp/src/signalrclient/http_client.h

websocket_client: https://github.com/aspnet/AspNetCore/blob/28ccc03fb90499425a9a3deb0c066e8624fff23a/src/SignalR/clients/cpp/src/signalrclient/default_websocket_client.h

transport: https://github.com/aspnet/AspNetCore/blob/28ccc03fb90499425a9a3deb0c066e8624fff23a/src/SignalR/clients/cpp/src/signalrclient/transport.h

As a reminder, the http_client and websocket_client are the things we're wanting to be replaceable. By default we will have implementations that use cpprestsdk.

BrennanConroy · 2019-03-15T00:50:15Z

Ping, any feedback?

No feedback is fine too, just want people to take a look :)

One thing I've been thinking about is accepting the http_client as a unique_ptr to the hub_connection, that way the lifetime is well known and there wont be any weird issues with the http_client and connection being in the same scope but destructors running at different times causing dangling pointer usage.

analogrelay

Looks pretty good

analogrelay · 2019-03-15T03:47:28Z

src/SignalR/clients/cpp/src/signalrclient/http_client.h

+    {
+    public:
+        int status_code = 0;
+        std::string content;


Are we going to support SSE? If not, and I think we can probably do without it, this should be fine. If we do want to support it, we'll need this to be a stream of some kind.

We just need to make sure that it can be added.

We talked about using ostream for this and istream for the request content. The problem is that only offers blocking and polling APIs. The alternative is using Asio's AsyncRead/WriteStream.

Since we're only aiming for source compatibility initially instead of some stable C ABI, we can probably just add an async stream field to http_request/response later and gracefully skip SSE if the http_client that doesn't use the field. We'd need a new send overload that calls the callback prior to buffering the entire response anyway.

src/SignalR/clients/cpp/src/signalrclient/http_client.h

analogrelay · 2019-03-15T03:51:11Z

src/SignalR/clients/cpp/src/signalrclient/websocket_client.h


-        virtual ~websocket_client() {};
+        virtual void receive(std::function<void(std::string, std::exception_ptr)> callback) = 0;


From the naming and structure I presume this calls the callback once for each call to receive, when a single frame is received? Another alternative would be a on_recieved API that registers a callback which will be called once for each frame that arrives (and we'd do the looping ourselves).

analogrelay · 2019-03-15T03:52:03Z

src/SignalR/clients/cpp/src/signalrclient/transport.h


-        logger m_logger;
+        virtual void on_receive(std::function<void(std::string, std::exception_ptr)> callback) = 0;


I'd expect to use the same API in websocket_client as we do here, so maybe we should change websocket_client to use on_receive as well...

The question is, do we expect people who provide their own websocket impl to do the receive loop and call our callback, or do we do the receive loop in the transport and just call receive on their impl?

analogrelay · 2019-03-15T03:53:13Z

One thing I've been thinking about is accepting the http_client as a unique_ptr to the hub_connection, that way the lifetime is well known and there wont be any weird issues with the http_client and connection being in the same scope but destructors running at different times causing dangling pointer usage.

Did you say... ownership?

analogrelay · 2019-03-15T03:53:37Z

I do think that's a good idea though. Big fan of unique_ptr when we feel like it makes sense, and I think it makes sense here.

src/SignalR/clients/cpp/src/signalrclient/default_http_client.cpp

halter73 · 2019-03-15T06:31:25Z

src/SignalR/clients/cpp/src/signalrclient/http_client.h

+        http_method method;
+        std::map<std::string, std::string> headers;
+        std::string content;
+        std::chrono::seconds timeout;


Maybe timeout should be a separate parameter on send. Or better yet, define a single-method CTS-like class that we return from send instead.

halter73 · 2019-03-15T06:42:26Z

src/SignalR/clients/cpp/src/signalrclient/websocket_transport.cpp

+
+                    if (!cts.get_token().is_canceled())
+                    {
+                        transport->receive_loop(cts);


Could this stack dive if enough messages were buffered in memory that websocket_client->receive repeatedly called its callback inline?

Definitely could. I believe we talked about this and decided to do it this way for now and re-evaluate later if we need to dispatch.

If there's a risk, I figure it's easy enough to turn this into a trampoline to avoid the problem. I think it would look something like this.

auto calling_receive = true; while (calling_receive) { websocket_client->receive([weak_transport, cts, logger, websocket_client, calling_receive](std::string message, std::exception_ptr exception) { // ... if (!cts.get_token().is_canceled()) { if (calling_receive) { calling_receive = false } else { transport->receive_loop(cts); } } }); calling_receive = !calling_receive; }

Something like that would work, but we still need to jump off the main thread for the start of the receive loop in order to avoid blocking start. Right now I'm relying on the transport dispatching, like we talked about in the meeting.

What's the main thread? Whatever thread you call connection.start() on?

Do we jump off the main thread currently before calling websocket_client->receive()? Or does this design prevent us from jumping off the main thread in the future?

I would think the trampoline is strictly better than the recursive version because it avoids potentially stack diving.

What's the main thread? Whatever thread you call connection.start() on?

Correct.

Do we jump off the main thread currently before calling websocket_client->receive()? Or does this design prevent us from jumping off the main thread in the future?

We don't do anything threading related yet, except in the http_client and websocket_client as those use pplx. We can add it whenever though.

I would think the trampoline is strictly better than the recursive version because it avoids potentially stack diving.

I agree.

src/SignalR/clients/cpp/src/signalrclient/websocket_transport.cpp

src/SignalR/clients/cpp/samples/HubConnectionSample/HubConnectionSample.cpp

jkotalik · 2019-03-20T00:24:53Z

src/SignalR/clients/cpp/src/signalrclient/connection_impl.cpp

    }

    std::shared_ptr<connection_impl> connection_impl::create(const std::string& url, trace_level trace_level, const std::shared_ptr<log_writer>& log_writer,
-        std::unique_ptr<web_request_factory> web_request_factory, std::unique_ptr<transport_factory> transport_factory)
+        std::unique_ptr<http_client> http_client, std::unique_ptr<transport_factory> transport_factory)


If someone passes a null http_client here, we should probably throw here.

Nah, https://github.com/aspnet/AspNetCore/pull/8420/files#diff-94c5397b9dcb8a27dcc5dd7ca166abe6R42 😛

Instead of passing nullptr here: https://github.com/aspnet/AspNetCore/pull/8420/files#diff-94c5397b9dcb8a27dcc5dd7ca166abe6R27, pass in https://github.com/aspnet/AspNetCore/pull/8420/files#diff-94c5397b9dcb8a27dcc5dd7ca166abe6R48

src/SignalR/clients/cpp/src/signalrclient/connection_impl.cpp

jkotalik · 2019-03-20T15:52:15Z

src/SignalR/clients/cpp/src/signalrclient/default_websocket_client.cpp

+            {
+                try
+                {
+                    task.get();


I think I need a bit more context on what this method is doing. Why do you need to complete the task provided here? Is the callback here only used for exception handling or is it required?

This is a point in time thing as we're still using pplx::task in our code.

What this is doing, is observing any exceptions from the completed task so we don't crash the process, and then forwarding the exception to the new callback based APIs

jkotalik · 2019-03-20T15:54:20Z

src/SignalR/clients/cpp/src/signalrclient/default_websocket_client.cpp

+                }
+                catch (...)
+                {
+                    callback(std::current_exception());


I'm not convinced this pattern for Try/Catch is good. Why not catch a std::exception& and pass the ref into the callback? That way you don't need to rethrow to get the exception.

jkotalik

LGTM from the perspective of non-signalR dev. IMO let's try to get this in sooner than later.

src/SignalR/clients/cpp/src/signalrclient/negotiate.cpp

src/SignalR/clients/cpp/src/signalrclient/websocket_transport.cpp

BrennanConroy · 2019-03-22T23:00:06Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=128810&view=logs

BrennanConroy · 2019-03-25T16:24:46Z

Test failures: https://github.com/aspnet/AspNetCore-Internal/issues/1756 and https://github.com/aspnet/AspNetCore-Internal/issues/2074

davidfowl · 2019-03-26T23:38:01Z

👏

BrennanConroy added the area-signalr Includes: SignalR clients and servers label Mar 11, 2019

BrennanConroy requested review from halter73, mikaelm12, analogrelay and davidfowl March 11, 2019 21:50

analogrelay reviewed Mar 15, 2019

View reviewed changes

halter73 reviewed Mar 15, 2019

View reviewed changes

src/SignalR/clients/cpp/src/signalrclient/default_http_client.cpp Show resolved Hide resolved

halter73 reviewed Mar 15, 2019

View reviewed changes

src/SignalR/clients/cpp/src/signalrclient/websocket_transport.cpp Show resolved Hide resolved

BrennanConroy force-pushed the brecon/lowapi branch from d1e7157 to b79217f Compare March 19, 2019 16:13

BrennanConroy marked this pull request as ready for review March 19, 2019 23:14

BrennanConroy requested a review from jkotalik March 20, 2019 00:09

jkotalik reviewed Mar 20, 2019

View reviewed changes

BrennanConroy mentioned this pull request Mar 22, 2019

Remove pplx::task from public API #8747

Merged

jkotalik approved these changes Mar 22, 2019

View reviewed changes

src/SignalR/clients/cpp/src/signalrclient/negotiate.cpp Show resolved Hide resolved

src/SignalR/clients/cpp/src/signalrclient/websocket_transport.cpp Outdated Show resolved Hide resolved

src/SignalR/clients/cpp/src/signalrclient/websocket_transport.cpp Outdated Show resolved Hide resolved

BrennanConroy requested a review from Tratcher as a code owner March 25, 2019 16:31

BrennanConroy added 8 commits March 25, 2019 09:36

stash

17f1036

cleanup

418559a

http

7d2f900

transfer format

4f32a79

stash tests

388e405

stash

2197632

stash

a01c345

progress

cc5ba3c

BrennanConroy added 10 commits March 25, 2019 09:43

all tests

933c4c1

enum class

a61168e

clean

ca8bcd0

headers

065094e

compilation failures on mac

9d41aab

more cleanup

a24c117

fb

df96942

moving files

a48049c

fb

db41d8a

wrong queue

0cffafa

BrennanConroy force-pushed the brecon/lowapi branch from b1bf056 to 0cffafa Compare March 25, 2019 16:47

fix rebase

e091fde

analogrelay added this to the 3.0.0-preview4 milestone Mar 25, 2019

BrennanConroy merged commit 969c72a into master Mar 25, 2019

BrennanConroy deleted the brecon/lowapi branch March 25, 2019 22:13

BrennanConroy mentioned this pull request Mar 25, 2019

Public API review for SignalR C++ Client #8717

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C++ client low level API #8420

C++ client low level API #8420

BrennanConroy commented Mar 11, 2019

BrennanConroy commented Mar 15, 2019

analogrelay left a comment

analogrelay Mar 15, 2019

davidfowl Mar 15, 2019

halter73 Mar 15, 2019

analogrelay Mar 15, 2019

analogrelay Mar 15, 2019

BrennanConroy Mar 16, 2019

analogrelay commented Mar 15, 2019

analogrelay commented Mar 15, 2019 •

edited

Loading

halter73 Mar 15, 2019

halter73 Mar 15, 2019

BrennanConroy Mar 15, 2019

halter73 Mar 15, 2019

BrennanConroy Mar 15, 2019

halter73 Mar 15, 2019

BrennanConroy Mar 18, 2019

jkotalik Mar 20, 2019

BrennanConroy Mar 20, 2019

jkotalik Mar 20, 2019

jkotalik Mar 20, 2019

BrennanConroy Mar 20, 2019

jkotalik Mar 20, 2019

jkotalik left a comment

BrennanConroy commented Mar 22, 2019

BrennanConroy commented Mar 25, 2019

davidfowl commented Mar 26, 2019


		virtual ~websocket_client() {};
		virtual void receive(std::function<void(std::string, std::exception_ptr)> callback) = 0;


		logger m_logger;
		virtual void on_receive(std::function<void(std::string, std::exception_ptr)> callback) = 0;

C++ client low level API #8420

C++ client low level API #8420

Conversation

BrennanConroy commented Mar 11, 2019

BrennanConroy commented Mar 15, 2019

analogrelay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analogrelay commented Mar 15, 2019

analogrelay commented Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotalik left a comment

Choose a reason for hiding this comment

BrennanConroy commented Mar 22, 2019

BrennanConroy commented Mar 25, 2019

davidfowl commented Mar 26, 2019

analogrelay commented Mar 15, 2019 •

edited

Loading