Refactor NetDaemonHost.HandleNewEvent #339
Conversation
| } | ||
| }, token)); | ||
| } | ||
| observer.OnError(e); |
There was a problem hiding this comment.
I am not sure if this is actually the corrcet way tp handle errors in Observables. I think OnError should be calles when the producers causes an error, not the observer. The same patteren is repeated here a coupe of times slightly different.
There was a problem hiding this comment.
Ok well I need to revisit this again. Not sure. Was a quite a time ago I implemented
| foreach (var netDaemonRxApp in NetDaemonRxApps) | ||
| { | ||
| // Call the observable with no blocking | ||
| foreach (var observer in ((StateChangeObservable)netDaemonRxApp.StateChangesObservable).Observers) |
There was a problem hiding this comment.
I wanted to Create a property StateChangesObservables like I did for the other one, but here the netDaemonRxApp is used for logging. The other events do not log using the app so maybe that is not actually needed
| } | ||
| }, token)); | ||
| } | ||
| observer.OnError(e); |
There was a problem hiding this comment.
This one is even more complicated, in the catch call OnError in another try catch. If this is really needed (what I doubt) this behaviour can probably extracted in a wrapper for the observer. Otherwise It would be enough to just catch and maybe log all exceptions from eventhanders
There was a problem hiding this comment.
Yea if we gonna refactor the error handling we need to test it so we make sure all exceptions are logged and the process continue handle messages. But you are probably right.
|
@FrankBakkerNl thanks for this. Sorry for being late merging. Been busy times at work. I will check it tomorrow, there are some removed code I need to think about if it is ok. Looks good overall ! |
| } | ||
| }, token)); | ||
| } | ||
| observer.OnError(e); |
There was a problem hiding this comment.
Now I think I know why I did this design. This is so I will pass any exception back to app so apps can also handle exceptions or they would go silent if I remember correctly
| } | ||
| }, token)); | ||
| } | ||
| observer.OnError(e); |
There was a problem hiding this comment.
Yea if we gonna refactor the error handling we need to test it so we make sure all exceptions are logged and the process continue handle messages. But you are probably right.
* Refactor NetDaemonHost.HandleNewEvent * fix tests
Breaking change
Proposed change
Type of change
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: