Yortw / RSSDP

Really Simple Service Discovery Protocol - a 100% .Net implementation of the SSDP protocol for publishing custom/basic devices, and discovering all device types on a network.
http://yortw.github.io/RSSDP/
MIT License
288 stars 67 forks source link

Mono: Network systemd down set socket op #78

Closed jormenjanssen closed 6 years ago

jormenjanssen commented 6 years ago

It seems like RSSDP is trying to set the socket opts without try/catch support. It is not acceptable to crash a procces on a rssdp failure. A better options would be to log that it failed and continue further execution.

2018-02-01 07:31:38,562 [Threadpool worker] FATAL Riwo.Common.Logging.Log4NetLogger: Unhandled exception caught. Server will shutdown... 2018-02-01 07:31:38,563 [Threadpool worker] FATAL Riwo.Common.Logging.Log4NetLogger: - Exception: System.Net.Sockets.SocketException (0x80004005): Network subsystem is down at System.Net.Sockets.Socket.SetSocketOption (System.Net.Sockets.SocketOptionLevel optionLevel, System.Net.Sockets.SocketOptionName optionName, System.Object optionValue) [0x000f4] in <4485f58287054508b32f8da191b697e4>:0 at Rssdp.SocketFactory.SetMulticastSocketOptions (System.Net.Sockets.Socket retVal, System.Int32 multicastTimeToLive) [0x0003b] in <66322f45ffa543d09ec24c74491d0251>:0 at Rssdp.SocketFactory.CreateUdpMulticastSocket (System.Int32 multicastTimeToLive, System.Int32 localPort) [0x0007e] in <66322f45ffa543d09ec24c74491d0251>:0 at Rssdp.Infrastructure.SsdpCommunicationsServer.ListenForBroadcastsAsync () [0x00000] in <66322f45ffa543d09ec24c74491d0251>:0 at Rssdp.Infrastructure.SsdpCommunicationsServer.BeginListeningForBroadcasts () [0x00027] in <66322f45ffa543d09ec24c74491d0251>:0 at Rssdp.Infrastructure.SsdpDevicePublisherBase..ctor (Rssdp.Infrastructure.ISsdpCommunicationsServer communicationsServer, System.String osName, System.String osVersion, Rssdp.ISsdpLogger log) [0x000f6] in <66322f45ffa543d09ec24c74491d0251>:0 at Rssdp.Infrastructure.SsdpDevicePublisherBase..ctor (Rssdp.Infrastructure.ISsdpCommunicationsServer communicationsServer, System.String osName, System.String osVersion) [0x00009] in <66322f45ffa543d09ec24c74491d0251>:0 at Riwo.Rimote.Client.RimoteDevicePublisher..ctor () [0x0001a] in <bc4a46f4c5b04b2baf2b4cd9c6c5c882>:0 at Riwo.Rimote.Client.ClientRimoteDevice+<StartAdvertising>d__52.MoveNext () [0x00055] in <bc4a46f4c5b04b2baf2b4cd9c6c5c882>:0 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw () [0x0000c] in <91f864b43b384ee3a77fe736602bbe37>:0 at System.Runtime.CompilerServices.AsyncMethodBuilderCore+<>c.<ThrowAsync>b__6_1 (System.Object state) [0x00000] in <91f864b43b384ee3a77fe736602bbe37>:0 at System.Threading.QueueUserWorkItemCallback.WaitCallback_Context (System.Object state) [0x00007] in <91f864b43b384ee3a77fe736602bbe37>:0 at System.Threading.ExecutionContext.RunInternal (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state, System.Boolean preserveSyncCtx) [0x00071] in <91f864b43b384ee3a77fe736602bbe37>:0 at System.Threading.ExecutionContext.Run (System.Threading.ExecutionContext executionContext, System.Threading.ContextCallback callback, System.Object state, System.Boolean preserveSyncCtx) [0x00000] in <91f864b43b384ee3a77fe736602bbe37>:0 at System.Threading.QueueUserWorkItemCallback.System.Threading.IThreadPoolWorkItem.ExecuteWorkItem () [0x00021] in <91f864b43b384ee3a77fe736602bbe37>:0 at System.Threading.ThreadPoolWorkQueue.Dispatch () [0x00074] in <91f864b43b384ee3a77fe736602bbe37>:0 at System.Threading._ThreadPoolWaitCallback.PerformWaitCallback () [0x00000] in <91f864b43b384ee3a77fe736602bbe37>:0 2018-02-01 07:31:38,607 [ 1] INFO Riwo.Common.Logging.Log4NetLogger: Finished operation: Service:Startup in 897ms (result: success, code: ok)

Yortw commented 6 years ago

I agree the process should not shutdown in this case. Unfortunately I have not tested on mono (have not had suitable environment or time to do so) and was not aware this threw. When I have time I will attempt to patch. Thanks for the detailed error information.

Yortw commented 6 years ago

Hi,

When I initially reviewed this report I only glanced at the stack trace and had mistakenly assumed the error was occurring on a background thread where the application code was not able to catch it. On further inspection it would appear the error within RSSDP is being thrown from the constructor of the publisher class, and this will be returned to the application code that tried to create the instance. The application should have no problem catching this and handling it appropriately (whether that be a retry, notifying the user to check their connection, logging and continuing with a silent failure etc). Only the application can really decide how important this error is and how to handle it.

The error handling policy in RSSDP is that 'background errors', those that the application cannot catch because they occur on a background thread with no application code in the call stack, are logged/reported and the process continues as best it can. For errors that are returned 'synchronously' to application code via a throw in a call stack containing app code, the app code should catch the exception and handle it.

In this case it appears the application code (possibly in the Riwo.Rimote.Client.ClientRimoteDevice.StartAdvertising method - though I'll leave that determination to you) should catch and handle the error. At this point the application code has tried to create a publisher and failed - the application is able to determine this and decide how best to proceed. If RSSDP did log the error then you'd just be left with a publisher that wasn't working and no good/easy way to know that or do anything about it. In fact in this case it does not look like RSSDP is crashing the process, but rather RSSDP is notifying the client application code of an error, and the application code is not handling it, leading to a crash.

Given this, I do not intend to modify RSSDP. It is behaving by design in this specific case, and the application should use a try/catch to handle the error appropriately.

Thanks for reporting the issue though.

ramondeklein commented 6 years ago

Thanks for your support, but it was our own fault. We used an async method and although the call raised an exception in the initial synchronous code, it wasn't catched anymore.

Yortw commented 6 years ago

Glad you found a solution :)