[Bug] Create new IWebSocketFeed when reconnecting

[zaccharles]

This change is to fix a NullReferenceException which is thrown upon reconnecting.

The reason this exception is thrown is that WebSocket_Closed disposes the WebSocket (webSocketFeed) here. WebSocket.Dispose is setting it's TcpClientSession to null here. As a result, when we call Start and try to Open the web socket again here, we get the exception due to Client being null.

My change modifies our WebSocket class to take a Func<IWebSocketFeed> so that each time we call Start, we can make a new IWebSocketFeed. There is an open issue, kerryjiang/WebSocket4Net#99, about this preventing reuse. Other socket libraries suggest just creating a new client like this PR, which work for me in practice.

It'd be nice if this could be merged, and if a new NuGet package could be published to pick up other changes (like the WebSocketX events that were added).

Thanks!

[BradForsythe]

@zaccharles is correct. Since the "new WebSocketFeed" was removed from Start and created at the client. Dispose is not needed on close. The simplest solution is to remove the "dispose" from close.

[BradForsythe]

I just moved my production code to use this latest...it's a bit broken at the moment, hopefully this weekend it will be back up and able to test.

[zaccharles]

@BradForsythe If you just remove the Dispose call from Close, you get a different exception:

System.Exception: The socket is connected, you needn't connect again!
   at SuperSocket.ClientEngine.TcpClientSession.Connect(EndPoint remoteEndPoint)
   at WebSocket4Net.WebSocket.Open()
   at GDAXSharp.WebSocket.WebSocketFeed.Open() in G:\<removed>\GDAX.Api.ClientLibrary\GDAXSharp\WebSocket\WebSocketFeed.cs:line 47
   at GDAXSharp.WebSocket.WebSocket.Start(List`1 providedProductTypes, List`1 providedChannelTypes) in G:\<removed>\GDAX.Api.ClientLibrary\GDAXSharp\WebSocket\WebSocket.cs:line 73
   at GDAXSharp.WebSocket.WebSocket.WebSocket_Closed(Object sender, EventArgs e) in G:\<removed>\GDAX.Api.ClientLibrary\GDAXSharp\WebSocket\WebSocket.cs:line 213
   at WebSocket4Net.WebSocket.FireClosed()
   at WebSocket4Net.WebSocket.OnClosed()
   at WebSocket4Net.WebSocket.client_Error(Object sender, ErrorEventArgs e)
   at SuperSocket.ClientEngine.ClientSession.OnError(Exception e)
   at SuperSocket.ClientEngine.AuthenticatedStreamTcpSession.StartRead()
   at SuperSocket.ClientEngine.AuthenticatedStreamTcpSession.OnDataRead(IAsyncResult result)
   at System.Net.LazyAsyncResult.Complete(IntPtr userToken)
   at System.Net.LazyAsyncResult.ProtectedInvokeCallback(Object result, IntPtr userToken)
   at System.Net.Security._SslStream.ProcessFrameBody(Int32 readBytes, Byte[] buffer, Int32 offset, Int32 count, AsyncProtocolRequest asyncRequest)
   at System.Net.Security._SslStream.ReadFrameCallback(AsyncProtocolRequest asyncRequest)
   at System.Net.AsyncProtocolRequest.CompleteRequest(Int32 result)
   at System.Net.FixedSizeReader.CheckCompletionBeforeNextRead(Int32 bytes)
   at System.Net.FixedSizeReader.ReadCallback(IAsyncResult transportResult)
   at System.Net.LazyAsyncResult.Complete(IntPtr userToken)
   at System.Net.ContextAwareResult.CompleteCallback(Object state)
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Net.ContextAwareResult.Complete(IntPtr userToken)
   at System.Net.LazyAsyncResult.ProtectedInvokeCallback(Object result, IntPtr userToken)
   at System.Net.Sockets.BaseOverlappedAsyncResult.CompletionPortCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* nativeOverlapped)
   at System.Threading._IOCompletionCallback.PerformIOCompletionCallback(UInt32 errorCode, UInt32 numBytes, NativeOverlapped* pOVERLAP)

[zaccharles]

I haven't looked into WebSocket4Net, but it looks like something isn't being closed properly or recreated when needed. I suppose it would be ideal if the WebSocket could be reused instead of recreated. That doesn't seem to be the case, so for now it's proably best to continue disposing it and making a new one.

[BradForsythe]

@zaccharles Yes. My project is back up. And I definitely get all kinds of weirdness when just removing dispose. I have an external implementation of Websocket4Net and it is possible to open and close without having to recreate. Looking into this a little later...time to eat!

[dougdellolio]

thanks @zaccharles!

@BradForsythe let me know what you find before I merge this fix.

[BradForsythe]

@zaccharles Your method to create each time we open and close is workable. @dougdellolio I have been able to implement open and closing the socket without recreating. Kind of using your original approach but without disposing and recreating, and not using the IWebSocket interface. I am thinking recreating the resources is more time consuming, but maybe not a big deal. For me ... I have a need to stop and start the socket when I detect errors or my "mini L2 books" need to resync. This can be done with separate methods to unsubscribe and resubscribe and not have to start and stop sockets. Even better!

To summarize:
Two options seem to work fine:
A. @zaccharles approach to pass in the recreate function or ...
B. Keep Websocket4Net inside Websocket and allow to open and close without disposing and without recreating.

Either way, new methods would be good to unsubscribe and resubscribe. Still there are times throughout the day GDAX decided to close your connection, or for whatever reason the subscriptions are not all accepted. Opening and closing the socket cures this. In these cases speed is likely not an issue.

Well, time to get something to eat for real now!

[dougdellolio]

going to merge this and release new version. thanks @BradForsythe and @zaccharles!