Public API review for SignalR C++ Client

[analogrelay]

Epic #5301

We need to do a public API review for the SignalR C++ client to make sure we're happy with it.

[BrennanConroy]

Couple notes from PR reviews:
http_request timeout - should we have a separate parameter on send instead, or CTS-like class we return from send?
websocket_transport - "trampoline" instead of recursion in receive loop (would prevent stack overflow from bad code), #8420 (comment)
Threading - Currently relying on the websocket/http stack to do threading, we should decide on who owns threading and how to do it
websocket_client - on_receive vs receive, receive means we control the loop, on_receive means websocket_client providers need to handle the loop
set_disconnected - the callback should probably accept an exception

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[davidfowl]

@BrennanConroy are these issues still relevant or are we tracking the client work separately?

[BrennanConroy]

hub_connection_builder.h

namespace signalr
{
    class hub_connection_builder
    {
    public:
        hub_connection_builder(std::string url);

        ~hub_connection_builder();

        hub_connection_builder(const hub_connection_builder&) = delete;

        hub_connection_builder(hub_connection_builder&&) noexcept;

        hub_connection_builder& operator=(hub_connection_builder&&) noexcept;

        hub_connection_builder& operator=(const hub_connection_builder&) = delete;

        hub_connection_builder& configure_options(signalr_client_config config);

        hub_connection_builder& with_logging(std::shared_ptr<log_writer> logger, trace_level log_level);

        hub_connection_builder& with_websocket_factory(std::function<std::shared_ptr<websocket_client>(const signalr_client_config&)> websocket_factory);

        hub_connection_builder& with_http_client_factory(std::function<std::shared_ptr<http_client>(const signalr_client_config&)> http_client_factory);

        hub_connection_builder& skip_negotiation(bool skip = true);

        hub_connection_builder& with_messagepack_hub_protocol();

        std::unique_ptr<hub_connection> build();
    }
}

Usage

auto hub_connection = std::shared_ptr<signalr::hub_connection>(
    signalr::hub_connection_builder("http://localhost:5000/hub")
    .skip_negotiation()
    .build());

Talking points:

• with_messagepack_hub_protocol is exposed even if you didn't compile with messagepack enabled. Calling it would throw then. We can put it in an #ifdef but then user code would need to define the DEFINE in order to use the API.
  • Or we could look into exposing the hub_protocol class in which case we'd also make messagepack_hub_protocol public
• Should build() return a pointer or shared_ptr?
  • pointer likely makes a C-wrapper easier, but does require users to call delete when they are done with the connection, or to wrap the pointer in a shared_ptr themselves (edit: looked into C-wrapper a little and it likely doesn't care)
• .NET client uses many WithUrl overloads to configure http options

hub_connection.h

namespace signalr
{
    class hub_connection
    {
    public:
        ~hub_connection();

        hub_connection(const hub_connection&) = delete;

        hub_connection& operator=(const hub_connection&) = delete;

        hub_connection(hub_connection&&) noexcept;

        hub_connection& operator=(hub_connection&&) noexcept;

        void start(std::function<void(std::exception_ptr)> callback) noexcept;
        void stop(std::function<void(std::exception_ptr)> callback) noexcept;

        connection_state get_connection_state() const;
        const std::string& get_connection_id() const;

        void on_disconnected(std::function<void (std::exception_ptr)> disconnected_callback);

        void on(const std::string& method_name, std::function<void (const std::vector<signalr::value>&)> handler);

        void invoke(const std::string& method_name, const std::vector<signalr::value>& arguments = std::vector<signalr::value>(), std::function<void(const signalr::value&, std::exception_ptr)> callback = [](const signalr::value&, std::exception_ptr) {}) noexcept;

        void send(const std::string& method_name, const std::vector<signalr::value>& arguments = std::vector<signalr::value>(), std::function<void(std::exception_ptr)> callback = [](std::exception_ptr) {}) noexcept;
    }
}

Usage:

std::shared_ptr<hub_connection> connection;
connection->on("Receive", [](const std::vector<signalr::value>& args)
{
    args[0].as_double();
    args[1].as_map();
});

connection->on_disconnected([](std::exception_ptr exception)
{
    std::cout << "Connection closed" << std::endl;
});

connection->start([](std::exception_ptr exception)
{
    if (exception != std::nullptr)
    {
        try
        {
            std::rethrow_exception(exception);
        }
        catch (const std::exception& ex)
        {
            std::cout << ex.what() << std::endl;
        }
    }
});

connection->invoke("Send", std::vector<signalr::value> { "Test", 10 }, [](const signalr::value& args, std::exception_ptr exception)
{
    args[0].as_string();
});

connection->stop([](std::exception_ptr exception)
{
});

• what about cancellation of invoke (currently only supported in .NET client)

signalr_value.h

namespace signalr
{
    enum class value_type
    {
        map,
        array,
        string,
        float64,
        null,
        boolean,
        binary
    };

    class value
    {
    public:
        value();

        value(std::nullptr_t);

        value(value_type t);

        value(bool val);

        value(double val);

        value(const std::string& val);

        value(std::string&& val);

        value(const char* val);

        value(const char* val, size_t length);

        value(const std::vector<value>& val);

        value(std::vector<value>&& val);

        value(const std::map<std::string, value>& map);

        value(std::map<std::string, value>&& map);

        value(const std::vector<uint8_t>& bin);

        value(std::vector<uint8_t>&& bin);

        value(const value& rhs);

        value(value&& rhs) noexcept;

        ~value();

        value& operator=(const value& rhs);

        value& operator=(value&& rhs) noexcept;

        bool is_map() const;

        bool is_double() const;

        bool is_string() const;

        bool is_null() const;

        bool is_array() const;

        bool is_bool() const;

        bool is_binary() const;

        double as_double() const;

        bool as_bool() const;

        const std::string& as_string() const;

        const std::vector<value>& as_array() const;

        const std::map<std::string, value>& as_map() const;

        const std::vector<uint8_t>& as_binary() const;

        value_type type() const;
    }
}

log_writer.h

namespace signalr
{
    class log_writer
    {
    public:
        // NOTE: the caller does not enforce thread safety of this call
        virtual void write(const std::string &entry) = 0;

        virtual ~log_writer() {}
    };
}

Usage:

hub_connection_builder("").with_logging(std::make_shared<my_logger>(), trace_level::debug);

class my_logger : public log_writer
{
public:
    virtual void write(const std::string& entry) override
    {
        std::cout << entry << std::endl;
    }
}

trace_level.h

namespace signalr
{
    enum class trace_level : int
    {
        verbose = 0,
        debug = 1,
        info = 2,
        warning = 3,
        error = 4,
        critical = 5,
        none = 6,
    };
}

• We could do categories via a flags enum instead/in addition to level
  • hub_connection | transport (skips connection logs, etc.)

signalr_client_config.h

namespace signalr
{
    class signalr_client_config
    {
    public:
        signalr_client_config();

        const std::map<std::string, std::string>& get_http_headers() const noexcept;

        void set_http_headers(const std::map<std::string, std::string>& http_headers);

        void set_scheduler(std::shared_ptr<scheduler> scheduler);
        std::shared_ptr<scheduler> get_scheduler() const noexcept;

        void set_handshake_timeout(std::chrono::milliseconds);
        std::chrono::milliseconds get_handshake_timeout() const noexcept;

        void set_server_timeout(std::chrono::milliseconds);
        std::chrono::milliseconds get_server_timeout() const noexcept;

        void set_keepalive_interval(std::chrono::milliseconds);
        std::chrono::milliseconds get_keepalive_interval() const noexcept;
    }
}

• Extra options for cpprestsdk are carried over from legacy client
• The #ifdef pattern is ugly because it requires users to define the variable in order to consume the methods which isn't obvious
• We could also remove the options and wait for user feedback, and maybe add first-class support for certain features
• Non-const get_http_headers could be removed, would resolve copy concerns so we don't modify user settings
• In other clients we have http_connection_options and this would be settable via with_url

http_client.h

namespace signalr
{
    enum class http_method
    {
        GET,
        POST
    };

    // created by signalr and passed to http_client impls
    class http_request final
    {
    public:
        http_request()
            : method(http_method::GET), timeout(std::chrono::seconds(120))
        { }

        http_method get_method() const;

        const std::map<std::string, std::string>& get_headers() const;

        const std::string& get_content() const;

        std::chrono::seconds get_timeout() const;
    };

    // created by http_client impls, consumed by signalr
    class http_response final
    {
    public:
        http_response(); // default 200 status code?
        http_response(int code, const std::string& content);
        http_response(int code, std::string&& content);

        http_response(http_response&& rhs);
        http_response(const http_response& rhs);

        http_response& operator=(const http_response& rhs);

        http_response& operator=(http_response&& rhs) noexcept;
    };

    class http_client
    {
    public:
        virtual void send(const std::string& url, http_request& request,
            std::function<void(const http_response&, std::exception_ptr)> callback, cancellation_token token) = 0;

        virtual ~http_client() {}
    };
}

• implemented by users who don't want the default cpprestsdk http implementation
• not binary vs. text aware (think LongPolling in the future) should we change that?

websocket_client.h

namespace signalr
{
    class websocket_client
    {
    public:
        virtual ~websocket_client() {};

        virtual void start(const std::string& url, transfer_format transfer_format, std::function<void(std::exception_ptr)> callback) = 0;

        virtual void stop(std::function<void(std::exception_ptr)> callback) = 0;

        virtual void send(const std::string& payload, std::function<void(std::exception_ptr)> callback) = 0;

        virtual void receive(std::function<void(const std::string&, std::exception_ptr)> callback) = 0;
    };
}

• implemented by users who don't want the default cpprestsdk websocket implementation
• consider using std::vector<unit8_t> instead of std::string
  • Today we just cast std::string to uint8_t* when using binary

transfer_format.h

namespace signalr
{
    enum class transfer_format
    {
        text,
        binary
    };
}

• exposed in websocket_client so it knows whether to use text or binary framing

connection_state.h

namespace signalr
{
    enum class connection_state
    {
        connecting,
        connected,
        disconnected
    };
}

scheduler.h

namespace signalr
{
    typedef std::function<void()> signalr_base_cb;

    struct scheduler
    {
        virtual void schedule(const signalr_base_cb& cb, std::chrono::milliseconds delay = std::chrono::milliseconds::zero()) = 0;

        virtual ~scheduler() {}
    };
}

cancellation_token.h

namespace signalr
{
    class cancellation_token_source;

    class cancellation_token
    {
    public:
        void register_callback(std::function<void()> callback);

        bool is_canceled() const;
    };
}

• only used by implementors of http_client currently

transport_type.h

namespace signalr
{
    enum class transport_type
    {
        websockets
    };
}

• not exposed anywhere currently, could remove

---

• make types final? (sealed)
• noexcept, should we only apply it to ctor/move ctor/copy ctor?
  • if a method is noexcept and ends up throwing it calls std::terminate, so we need to be really confident it won't throw (don't trust user code, etc.)
  • perf optimizations for using noexcept mainly comes from ctors
  • noexcept does declare intent though
  • https://azure.github.io/azure-sdk/cpp_introduction.html#cpp-design-naming-functions-noexcept

  YOU SHOULD declare all functions that can never throw exceptions noexcept.

[halter73]

API Review Notes:

1. We are limited to C++11 in order to support environments without the latest and greatest (e.g. game consoles).
2. What about STL? VCPKG should help avoid issues with different versions of the STL.
3. We should consider header file names. Let's update the usage examples with #include ...
4. hub_connection_builder.create(string) vs withUrl(). create(string) seems fine.
5. Should build() return a pointer or shared_ptr? We think pointer is fine. You can create a shared_ptr if you want.
6. Should hub_connection_builder.create return a pointer? Does hub_connection_builder need a copy constructor if it's mostly tracking references so it's not really a copy?
7. Does with_logging need a shared_ptr? It feels safer. Logging can happen in the background.
8. Let's try removing the CPPRESTSDK-specific APIs from signalr_client_config and see if we get pushback. If something like proxy support is really needed, we can consider adding support to http_client.h.
9. Name with_websocket_factory parameter websocket_factory instead of factory.
10. Do we want to support building a hub_connection_builder more than once? Not for now.
11. Let's have with_websocket_factory and with_http_client_factory take std::function<websocket_client*>/std::function<http_client*> instead of a shared_ptr.
12. Is there an #ifdef for with_messagepack_hub_protocol? Inside of it yes. It's not really discoverable if we remove the public API by default. It will throw at runtime if you're missing the define. But the API is always available.

TBC

[AaronRobinsonMSFT]

5. Should build() return a pointer or shared_ptr? We think pointer is fine. You can create a shared_ptr if you want.

I would instead return std::unique_ptr<T>. The ownership of a naked pointer is in question. Providing std::unique_ptr<T> helps avoid that ambiguity.

make types final? (sealed)

Yes. This improves optimization opportunities.

I'd also recommend referring to https://github.com/isocpp/CppCoreGuidelines.

Taking some time to audit this API and how it works with the GSL might also be worthwhile.

[BrennanConroy]

I would instead return std::unique_ptr<T>

Wouldn't this make hub_connection a lot harder to use from different threads/places in code? Someone would need to place it in a global/another class and then use it via some methods on their class.

It would make it harder (impossible) to create a reference cycle via lambda captures (std::shared_ptr) though which is nice.

[AaronRobinsonMSFT]

Wouldn't this make hub_connection a lot harder to use from different threads/places in code? Someone would need to place it in a global/another class and then use it via some methods on their class.

Based on the statement, "We think pointer is fine. You can create a shared_ptr if you want.", there is an expectation for users to take a raw pointer and create a shared_ptr<T>, right? In the current form you are saying "here is a raw pointer with no assumptions about ownership, do as you please". That is wrong because if a user can immediately wrap the raw pointer it implies the pointer is owned by the caller. However, if you provide unique_ptr<T>, then the caller knows they own it and can do as they please safely. The shared_ptr<T> ctor accepts an R-value reference of a unique_ptr<T> specifically to indicate these ownership semantics and does it explicitly.

There should be no reason to use raw pointers at an API level in C++14. In C++11 there are a few cases, but none jumped out to me in the above API. Working through some use cases with the GSL would help with some of this.

[BrennanConroy]

The shared_ptr<T> ctor accepts an R-value reference of a unique_ptr<T>

Ah, that's helpful.

Working through some use cases with the GSL would help with some of this.

This seems difficult since the repo says it assumes C++14 or higher... we're stuck targeting C++11

[AaronRobinsonMSFT]

This seems difficult since the repo says it assumes C++14 or higher... we're stuck targeting C++11

Oh, I missed that. Sigh... Reviewing the core guidelines is worth at least an hour or two though.

[halter73]

API Review Notes (Cont.):

1. All copy and move constructors should have noexcept;.
2. We like the chaining of the hub_connection_builder methods.
3. If we are copying the std::map, just pass it by value unless we're processing the data inline.
   • Same goes for configure_options(const signalr_client_config& config). It should be configure_options(signalr_client_config config).
4. If you build a copy of a hub_connection_builder, are all copies "built" meaning they cannot be built again?
   • Let's change create(string) to a ctor and get rid of the copy constructor.
5. Do we need a public API to set HTTP headers if only SignalR itself sets them? No. http_request should be read-only and http_response can be write-only.
6. Why both get_http_headers() const noexcept; and std::map<std::string, std::string>& get_http_headers() noexcept;.
7. Should we even allow the transport to modify the signalr_client_config. It cannot because we pass it to the transport by const.
8. The non-const std::map<std::string, std::string>& get_http_headers() should be removed.
9. Is the scheduler from const std::shared_ptr<scheduler>& get_scheduler() const noexcept; being stored? Yes. Let's just make it std::shared_ptr<scheduler> get_scheduler() const noexcept;. No const references for returns (probably).
10. std::function<std::shared_ptr<websocket_client>(const signalr_client_config&)> websocket_factory should be using unique_ptr just like hub_connection_builder.build().
11. Always take std::function by value if you are storing it after the call.
12. Remove all the public constructors and assignment operators from hub_connection since we're now handing out a unique_ptr.
13. Let's remove all the typedefs for functions like method_invoked_handler.
14. std::exception_ptr is a bad API. We could pass a copy of the exception object itself but we'd lose info like the stack trace. Or... std::function<void(std::exception_ptr)> callback could be std::function<void(const std::exception*)> callback.
15. Remove the __cdecl.
16. Rename set_disconnected to on_disconnected.
17. Is on additive? Yes. Let's make it the same for on_disconnected.
18. Do we need the ability to remove on callbacks? Maybe. We could theoretically add a registration return value later and add an off method.
19. const std::string& vs const char*? const std::string& is better.
20. We ❤️ [CPP] API sketch BrennanConroy/SignalR#1

[halter73]

API Review Notes:

1. We love the template demo that builds on signalr_value.
2. Use *.hpp instead of *.h for all header files since we're using C++ even if it is header-only with no code.
3. Use < instead of " for includes.
4. Let's stick with unique_ptr for with_websocket_factory and with_http_client_factory. We're convinced we can make the tests work somehow.
5. Friend classes are cool!
6. Let's mark setters in signalr_client_config with noexcept. Do a noexcept pass.
7. Can we replicate std::variant or use it for signalr_value? No. It's C++14 and greater and the index API is unnecessary for a custom tagged union.
8. Pass parameters by value instead of reference to value constructors since we're just copying in the constructor anyway.
9. Remove value(const char* val). It's not too hard to have the user convert to std::string themselves.
10. value_type.null is a unique concept because we don't know the type we're extracting to. Given null, is_string will return false, as_string will throw, etc... We could return a unique_ptr from as_string to return "null", but this seems unwieldy. Custom converters can manually check is_null for nullable properties.
11. Let's move value_type into value and rename it to type.
12. Enums should not have : int
13. Should the 0 log_level be none to be more consistent with C++ libraries? It's probably better to align with Microsoft.Extensions.Logging LogLevel, but we should think about it.
14. Do we need categories for logs? Can we use less log levels? Error, debug, none?
15. Let's rename trace_level to log_level.
16. How do we flow the log writer and logger config to the transport and http client? Should we add it to signalr_client_config? Or should we force people to wire the logger themselves to custom transports and client?
17. All destructors should be marked noexcept.
18. Can we put http_method instead http_request? Would we rename it to just method if it's nested? Probably.
19. Can websocket_client be turned into an arbitrary transport? It's really close. The registration would have to change so with_websocket_factory is now called with_transport_factory and could take transport_type as the first parameter.
20. We assume the std::string used by http_response are UTF-8 encoded chars. Can we make it Vector<uint8_t>? Is there away to avoid copying. If the only callback reading the buffer is internal code, maybe it's okay to just take a pointer and length and promise to do any copying we need in the callback.
21. Don't block on read_to_end in the default_websocket_client!

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.