SignalR C++ Client Clean-up

[analogrelay]

Epic #5301

• [x] Remove unrelated tests
• [x] Remove remnants of classic ASP.NET protocol
• [ ] Resolve "TODO" comments
• [ ] Normalize log and exception messages (Capitalization and full stops)

[mmarinchenko]

Hi! May I add some considerations here? If this is not the best place for that please direct me to some other place where it would be more appropriate.

BACKGROUND

I'm trying to integrate some SignalR C++ Client code to our pet game project which uses Unreal Engine. UE4 uses paranoid warning level while compiling sources and treat warnings as errors. It also still depends on old gnustl version (GCC 4.9) on Android (but this is another story...).

It's especially interesting for us to try to integrate SignalR C++ Client code using UE's implementations of http/websocket clients, json library, build system, etc. instead of cpprestsdk, boost, cmake, etc.

I have no much expertise in C++ so my considerations easily can be irrelevant. If so feel free to ignore them :)

CONSIDERATIONS

1. The existence of make_unique.h file causes ambiguous function error when compiling with Clang for Linux (or with GCC 4.9 for Android - I may be wrong here because I don't remember exactly). I deleted this file and the error is gone on all platforms which I tried;

• Windows + MSVC (VS 2019 v16.4.4)
• Linux + Clang 8.0.1 (bundled with UE)
• iOS SDK 13.2
• Android NDK r17c (GCC 4.9)

So is it actually needed? I may add that old gnustl on Android lacks std:to_string(). But as I can see both functions are parts of C++11, so it also would be great to add list of supported compiler versions in repository main README.

2. There is one place in whole project where dynamic_cast<>() is used. connection_impl.cpp file:

                        catch (const std::exception& e)
                        {
                            auto canceled = dynamic_cast<const canceled_exception*>(&e);
                            if (canceled)
                            {
                                connection->m_logger.log(trace_level::info,
                                    "starting the connection has been canceled.");
                            }
                            else
                            {
                                connection->m_logger.log(trace_level::errors,
                                    std::string("connection could not be started due to: ")
                                    .append(e.what()));
                            }

It is possible to compile SignalR C++ Client without RTTI if we remove this special dynamic_cast<>() case, enable exceptions and link to UE's http, json, etc. libs which are also compiled without RTTI. Will this lead to any problems with exceptions behaviour at runtime? Is this special case so important to mandate RTTI?

3. At the very end of json_hub_protocol.cpp file the return statement uses std::move(), which prevents copy elision:

            // Future protocol changes can add message types, old clients can ignore them
            // TODO: null
            break;
        }

        return std::move(value);
    }
}

4. log_writer has virtual method:

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

and derived type:

    class trace_log_writer : public log_writer
    {
    public:
        void write(const std::string &entry) override;
    };

So log_writer should also have virtual destructor.

5. signalr_value has union with complex types:

        union storage
        {
            bool boolean;
            std::string string;
            std::vector<value> array;
            double number;
            std::map<std::string, value> map;

            storage() {}
            ~storage() {}
        };

This causes MSVC warnings 4582 and 4583 (link) because union shares the memory between fields and doesn't know how to create/destroy complex objects. Potential memory leak?

6. Initialization list reordering. There are three:

• in hub_connection_impl constructor m_handshakeReceived must be placed before m_disconnected;
• in websocket_transport constructor: -- m_signalr_client_config must be placed before m_receive_loop_cts; -- m_process_response_callback must be placed before m_close_callback.

---

P.S. Header inclusion order is a mess :)

[BrennanConroy]

The existence of make_unique.h file causes ambiguous function error when compiling

We haven't seen this, but can take a look at removing the file.

* Android NDK r17c (GCC 4.9)

I'm not going to pretend to know anything about Android NDK, but a quick search shows that you should probably not use GCC and switch to libc++ as GCC has been deprecated in that version of the NDK.

It is possible to compile SignalR C++ Client without RTTI

We can take a look at this especially if there is only 1 place where we use it currently.

The rest of your comments are about some warnings that we haven't cleaned up yet and will work on once we decide how to ignore warnings from external headers so we don't have hundreds of non-actionable warnings.

[mmarinchenko]

Thanks for the answer @BrennanConroy

You are right about Android NDK. The problem here is that UE depends on gnustl on Android) I.e. r17c is latest version of NDK which may be used with UE. This is not your problem of course, that's why I suggested to add list of supported compiler versions in README. I believe this would be very convenient for potential users of your code.

Some official support for non-RTTI compilation (in case of excluding cpprestsdk and boost) from your side would be very helpful, thanks!

[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]

Yes, tracking the C++ work with these issues.