Move Cpp Client to main repo #1703

BrennanConroy · 2018-03-23T22:33:05Z

No description provided.

analogrelay

Presuming no code changes since the last PR on the old repo and that it isn't wired in to the build yet, I'm good.

analogrelay · 2018-03-23T22:42:26Z

clients/cpp/README.md

+
+This folder contains a C++ client for ASP.NET Core SignalR.
+
+**This is an experimental project right now**


Let's make it even clearer and add "There are no plans to ship this client at this time"

analogrelay · 2018-03-23T22:42:42Z

clients/cpp/samples/ClientSample/ClientSample.vcxproj

@@ -0,0 +1,148 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project DefaultTargets="Build" ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <ItemGroup Label="ProjectConfigurations">


JamesNK · 2018-03-23T22:47:09Z

clients/cpp/samples/ClientSample/ClientSample.vcxproj

+  </ItemGroup>
+  <PropertyGroup Label="Globals">
+    <VCProjectVersion>15.0</VCProjectVersion>
+    <ProjectGuid>{BEC3FEA2-DD32-4B55-B27F-64A5F3DBBB46}</ProjectGuid>


Flashbacks D:

BrennanConroy · 2018-03-23T22:47:44Z

OSX 10.12, Ubuntu 16.04, and Windows all pass, so it must be good

Eilon · 2018-03-23T22:54:01Z

So we don't need the other repo?

BrennanConroy · 2018-03-23T22:55:24Z

Correct, but I still need to copy the issues over before you delete it

moozzyk

I actually think you would get further and quicker if you take the old client and remove things that are not needed.
Also, you will get quite a lot of test coverage for free.

moozzyk · 2018-03-24T04:16:09Z

clients/cpp/samples/ClientSample/ClientSample.vcxproj

@@ -0,0 +1,148 @@
+<?xml version="1.0" encoding="utf-8"?>


Look at files in folder here: https://github.com/SignalR/SignalR-Client-Cpp/tree/dev/Build - they help avoid duplication. I don't want to go too far but I think you will want to copy a lot of this. Not only because they worked but they also use compiler settings required/recommended by security folks.

moozzyk · 2018-03-24T04:19:10Z

clients/cpp/src/signalrlib/HttpConnection.cpp

+        web::http::client::http_client client(url);
+
+        web::http::http_request request;
+        request.set_method(L"POST");


think cross plat from the very beginning or you will have a lot of work later. Start from using _XPLATSTR or _U for string literals

moozzyk · 2018-03-24T04:28:11Z

clients/cpp/src/signalrlib/HubConnection.cpp

+    {
+        mUrl = url;
+        mTransport = new WebSocketsTransport(mUrl);
+        mTransport->OnReceived([&](const utility::string_t& messages)


don't capture like this - this will crash very hard. I did not read the code very carefully but I think this lambda can outlive the connection and all your m* variables will cause segfaults. There is a reason while the old SignalR C++ client used shared_ptr all over the place (https://github.com/SignalR/SignalR-Client-Cpp/blob/dev/src/signalrclient/connection_impl.cpp#L96). Read more on this in my post https://blog.3d-logic.com/2014/12/28/c-async-development-not-only-for-for-c-developers-part-ii-lambda-functions-the-most-common-pitfalls/
(because you are using Casablanca consider reading entire series: https://blog.3d-logic.com/category/cpprestsdk/ - it is basically my learnings from writing the SignalR client. Sadly, I have never written the last two posts).

Yeah, I'm going to be using shared_ptr later, this was a quick, get things working

moozzyk · 2018-03-24T04:29:34Z

clients/cpp/src/signalrlib/WebSocketsTransport.cpp

+        }
+        else
+        {
+            //???


throw - invalid URL

davidfowl · 2018-03-24T07:57:26Z

clients/cpp/src/signalrlib/packages.config

@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="utf-8"?>
+<packages>
+  <package id="cpprestsdk.v140.windesktop.msvcstl.dyn.rt-dyn" version="2.9.1" targetFramework="native" />


dat package name.

davidfowl · 2018-03-24T07:58:33Z

clients/cpp/samples/ClientSample/main.cpp

@@ -0,0 +1,34 @@
+// Copyright (c) .NET Foundation. All rights reserved.


Why is the ClientSample folder pascal case?

davidfowl · 2018-03-24T07:59:06Z

clients/cpp/src/signalrlib/signalrlib.filters

@@ -0,0 +1,35 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">


Why signalrlib and not lib or just signalr?

actually should be libsignalr for mac and linux

analogrelay · 2018-03-27T16:52:03Z

Wow

So... I presume a lot of this is the old client copied over?

Eilon

I approve the third-party-notices change.

JamesNK · 2018-03-28T20:04:47Z

A lot of files are missing a copyright header

analogrelay · 2018-03-28T20:06:26Z

BOOM!

BrennanConroy · 2018-03-29T05:28:31Z

Which files are missing copyright?

JamesNK · 2018-03-29T05:38:14Z

A saw in a quick scan:

clients/cpp/samples/HubConnectionSample/HubConnectionSample.cpp
clients/cpp/samples/PersistentConnectionSample/PersistentConnectionSample.cpp
clients/cpp/samples/PersistentConnectionSample/stdafx.cpp
clients/cpp/samples/PersistentConnectionSample/stdafx.h
clients/cpp/samples/SignalRServer/ChatHub.cs
clients/cpp/samples/SignalRServer/EchoConnection.cs
clients/cpp/samples/SignalRServer/Properties/AssemblyInfo.cs

BrennanConroy · 2018-03-29T05:42:22Z

I guess we can put copyright in samples...

Eilon

Re-signing off on licenses. Thanks 😄

BrennanConroy · 2018-03-29T05:55:21Z

100% not related to this PR, but still interesting

The active test run was aborted. Reason: Unhandled Exception: Unhandled Exception: System.InvalidOperationException: Another continuation was already registered.
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.ThrowMultipleContinuationsException()
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.OnCompleted(Action`1 continuation, Object state, Int16 token, ValueTaskSourceOnCompletedFlags flags)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AwaitUnsafeOnCompleted[TAwaiter,TStateMachine](TAwaiter& awaiter, TStateMachine& stateMachine)
--- End of stack trace from previous location where exception was thrown ---
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
   at System.Threading.ThreadPoolWorkQueue.Dispatch()System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values.
Parameter name: state
   at System.Threading.Tasks.ValueTask`1.ValueTaskSourceTask.<>c.<.cctor>b__4_0(Object state)
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.InvokeContinuation(Action`1 continuation, Object state, Boolean forceAsync)
   at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.OnCompleted(SocketAsyncEventArgs _)
   at System.Net.Sockets.SocketAsyncEventArgs.FinishOperationAsyncFailure(SocketError socketError, Int32 bytesTransferred, SocketFlags flags)
   at System.Net.Sockets.SocketAsyncEventArgs.CompletionCallback(Int32 bytesTransferred, SocketFlags flags, SocketError socketError)
   at System.Net.Sockets.SocketAsyncEventArgs.TransferCompletionCallbackCore(Int32 bytesTransferred, Byte[] socketAddress, Int32 socketAddressSize, SocketFlags receivedFlags, SocketError socketError)
   at System.Net.Sockets.SocketAsyncContext.BufferMemoryReceiveOperation.InvokeCallback(Boolean allowPooling)
   at System.Net.Sockets.SocketAsyncContext.AsyncOperation.<>c.<TryCancel>b__17_0(Object o)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
   at System.Threading.ThreadPoolWorkQueue.Dispatch()

davidfowl · 2018-03-29T08:44:02Z

@BrennanConroy we're on a thread with the BCL about this.

Move Cpp Client to main repo (#1703)

Move Cpp Client to main repo

1e4a006

BrennanConroy requested review from analogrelay and davidfowl March 23, 2018 22:33

analogrelay approved these changes Mar 23, 2018

View reviewed changes

readme

40aca83

JamesNK reviewed Mar 23, 2018

View reviewed changes

moozzyk reviewed Mar 24, 2018

View reviewed changes

davidfowl reviewed Mar 24, 2018

View reviewed changes

Stash

f4e10f8

davidfowl mentioned this pull request Mar 26, 2018

Production use of SignalR for Asp.Net Core #429

Closed

BrennanConroy added 2 commits March 26, 2018 23:10

missing files

5d2f3b6

more missing files

6ca333d

License

730ee12

Eilon approved these changes Mar 28, 2018

View reviewed changes

fix up licenses

a05361a

Eilon approved these changes Mar 29, 2018

View reviewed changes

copyright

8b6c6d0

BrennanConroy merged commit 476d337 into release/2.1 Mar 29, 2018

BrennanConroy deleted the brecon/cppClient branch March 29, 2018 21:58

BrennanConroy added a commit that referenced this pull request Mar 29, 2018

Merge pull request #1771 from aspnet/release/2.1

ba1d4b7

Move Cpp Client to main repo (#1703)

am11 mentioned this pull request Oct 4, 2022

[linux-arm64] Random and rare runtime crash System.ArgumentOutOfRangeException (System.Net.Sockets) dotnet/runtime#72365

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Cpp Client to main repo #1703

Move Cpp Client to main repo #1703

BrennanConroy commented Mar 23, 2018

analogrelay left a comment

analogrelay Mar 23, 2018

analogrelay Mar 23, 2018

JamesNK Mar 23, 2018

BrennanConroy commented Mar 23, 2018

Eilon commented Mar 23, 2018

BrennanConroy commented Mar 23, 2018

moozzyk left a comment

moozzyk Mar 24, 2018

moozzyk Mar 24, 2018

moozzyk Mar 24, 2018

BrennanConroy Mar 24, 2018

moozzyk Mar 24, 2018

davidfowl Mar 24, 2018

davidfowl Mar 24, 2018

davidfowl Mar 24, 2018

moozzyk Mar 24, 2018

analogrelay commented Mar 27, 2018

Eilon left a comment

JamesNK commented Mar 28, 2018

analogrelay commented Mar 28, 2018

BrennanConroy commented Mar 29, 2018

JamesNK commented Mar 29, 2018

BrennanConroy commented Mar 29, 2018

Eilon left a comment

BrennanConroy commented Mar 29, 2018

davidfowl commented Mar 29, 2018


		This folder contains a C++ client for ASP.NET Core SignalR.

		This is an experimental project right now

		@@ -0,0 +1,34 @@
		// Copyright (c) .NET Foundation. All rights reserved.

		@@ -0,0 +1,35 @@
		<?xml version="1.0" encoding="utf-8"?>
		<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

Move Cpp Client to main repo #1703

Move Cpp Client to main repo #1703

Conversation

BrennanConroy commented Mar 23, 2018

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

BrennanConroy commented Mar 23, 2018

Eilon commented Mar 23, 2018

BrennanConroy commented Mar 23, 2018

moozzyk 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analogrelay commented Mar 27, 2018

Eilon left a comment

Choose a reason for hiding this comment

JamesNK commented Mar 28, 2018

analogrelay commented Mar 28, 2018

BrennanConroy commented Mar 29, 2018

JamesNK commented Mar 29, 2018

BrennanConroy commented Mar 29, 2018

Eilon left a comment

Choose a reason for hiding this comment

BrennanConroy commented Mar 29, 2018

davidfowl commented Mar 29, 2018