Sample Client/Server + Optional Sec-WebSocket-Protocol + No More Visual Studio Warnings

[MatrixManAtYrService]

Sample Client/Server + Optional Sec-WebSocket-Protocol + No More Visual Studio Warnings

I've been using Simple-WebSocket-Server for some time, and I've collected several changes. I will be leaving my current company in mid October, and won't be using Simple-WebSocket-Server after that. I figured it would be good to make those changes available in case you'd like them.

The changes fall in three categories:

Removing Visual Studio warnings

I understand that you don't recommend using Windows as a server. Neither do I. Most of my office develops on Windows, however, so it was necessary to get Simple-WebSocket-Server running on windows. We treat warnings as errors, so instead of silencing them, I went into your code and fixed them. All changes struck me as trivial. I also tested these changes on Linux--but haven't had the opportunity on any other OS.

Creating a sample client/server pair

In order to test your client against other servers, or your server with other clients, it was helpful to create separate test binaries so that we could mix/match. I packaged these in ./sample/ and wrote some documentation for them.

Adding support for Sec-WebSocket-Protocol

We ran across a server that required that this optional field be set (see #86). I added an optional call to SocketClient which allows the user to set this field. The server in this repo ignores the field (apart from including it in the response), but it is useful to be able to set the field when communicating with libwebsockets (which, I'm told, prefers to use the protocol instead of the http resource).

While doing this, I also learned that libwebsockets likes to respond a bit differently (still an HTTP 101, but then it says "Switching Protocols" instead of "Web Socket Protocol Handshake". I changed SocketClientBase so that it just looks for the 101, and ignores the message--which prevents an error.

Testing

These changes were tested with:

• Windows Server 2016 using Visual Studio 2017 (15.3.3)
• Ubuntu 16.04 using gcc 5.4.0

[eidheim]

Thank you. I'll have to look into this in more detail, but first some general comments/thoughts:

• I do not wish to support a platform specific compiler like MSVC (though this does not mean that I will not merge PR's that helps this project compile on MSVC). The major reasons being:
  • The compiler is lacking compared to clang and gcc. For instance, MSVC does not support -fno-access-control or equivalent flag that is sometimes needed when writing tests.
  • Clang is getting increasingly stable/usable on Windows. Chrome is already compiled with Clang for creating Windows binaries, and I question the future of MSVC (the compiler) if Clang becomes a valid option, after all, Clang has quite a higher number of developers.
• I do not wish to use -Wshadow since it encourages use of unnatural variable names.
• I'm open to adding -Wconversion, although I do not currently see the advantages that this warning flag would add to this project.
• Additional examples/samples would lead to more work, especially later when performing restructuring/improvements to the source code. Is there a possibility to change the current examples instead, without adding further complexity to the examples?
• Regarding adding Sec-WebSocket-Protocol, could we maybe add a variable in Client::Config, for instance CaseInsensitiveMultimap additional_header_fields, that the user can set. These header fields could then be used to add Sec-WebSocket-Protocol, but also for other kinds of information one would want to put into the request header.

Sorry for the brick wall of comments above, but it was a large PR:) Thank you for splitting it into several commits though, and your extensive comments!

[eidheim]

I completely forgot: you can supress the MSVC warnings from the headers by including them as system includes in your project.

[eidheim]

I just added the possibility to add additional headers in 06ce3a0.

Here is an example:

#include "client_ws.hpp"
#include "server_ws.hpp"

using namespace std;

using WsServer = SimpleWeb::SocketServer<SimpleWeb::WS>;
using WsClient = SimpleWeb::SocketClient<SimpleWeb::WS>;

int main() {
  // WebSocket (WS)-server at port 8080 using 1 thread
  WsServer server;
  server.config.port = 8080;

  auto &endpoint = server.endpoint["^/?$"];

  endpoint.on_open = [](shared_ptr<WsServer::Connection> connection) {
    cout << connection->header.find("Sec-WebSocket-Protocol")->second << endl;
  };

  thread server_thread([&server]() {
    server.start();
  });

  this_thread::sleep_for(chrono::seconds(1));

  WsClient client("localhost:8080");

  client.config.header = {{"Sec-WebSocket-Protocol", "some_protocol"}};

  client.start();

  server_thread.join();
}

// Output: 
// some_protocol

[eidheim]

I also cherry picked, and fixed, commit: e2a7650. The fix can be seen in 383e8ed. The emoji in the commit message was unintentional!

[MatrixManAtYrService]

MSVC vs CLANG

Your points about MSVC and CLANG make a lot of sense. I just wish I could convince the rest of the office to switch. I'll be leaving soon, so I guess their in charge of their own destiny now.

System Headers

Thinking back, I tried the system header thing. I think there was a bug in either CMake or Visual Studio that prevented it from working. I'll try it again--it's been a while.

Added Warnings

Some of the weird warning stuff comes from the fact that I grabbed a lot of that CMakeLists.txt file from one we were using elsewhere. Their inclusion was an oversight on my part.

Samples

I think that it's conceptually simpler to have a sample client that is separate from the sample server, it also makes integration testing easier. I haven't been using your examples (except for inspiration) for that reason.

On the other hand, being able to test data flow without a human in the middle banging on a keyboard--that sounds good too. My samples have served their purpose (to be provided to other teams)--I won't be sad if you don't want to adopt their extra complexity.

If you think others might want them, I could pull them into a separate repo which includes this one as a submodule. Whether you'd then want to link to that repo in your docs would be your call.

Optional Arbitrary Headers

That looks much better than what I had.

[eidheim]

Now that I have modernised the cmake files, I made an attempt to support MSVC in this commit that I just pushed to master: 3d18c3b

[eidheim]

Sorry about the close/reopen, I clicked the wrong button

[mpoleg]

I ran into the same warning C4309 on Visual Studio 17 for 64 bit configuration.
I made the same fix in my copy of this file.
But without replacing 126 + 128 with neither 0xFEu nor 0xFDu

[mpoleg]

The previous comment is about commit
Sample Client/Server + Optional Sec-WebSocket-Protocol + No More Visual Studio Warnings
related to file client_ws.hpp.