[BUG]

[BionicCode]

Description Library produces unexpected side effects when listening to the DeviceFound event and the event handler manipulates the UI. See Stack overflow post for details.

Since ALL exceptions are swallowed by an empty catch block (at least in the DeviceLocator class), the client code shows unexpected behavior in case of an exception, as it continues to execute in an unpredictable state. In case of the event handler is accessing the UI, a cross thread exception is expected to be thrown by the .Net framework.
The library should either let the exception reach the client code or/and marshal the event handler execution to the UI thread, by raising the even on the dispatcher: Application.Current.Dispatcher.InvokeAsync(() => OnDeviceFound?.Invoke(null, new DeviceFoundEventArgs(device)));. DeviceLocator line 154.

A library should never swallow exceptions as this can lead to unexpected side effects. The client must be given the choice to decide and handle exceptions individually.

Check library for more occurrences.

Expected behavior The library should either let the exception reach the client code or/and marshal the event handler execution to the UI thread, by raising the even on the dispatcher: Application.Current.Dispatcher.InvokeAsync(() => OnDeviceFound?.Invoke(null, new DeviceFoundEventArgs(device)));

Environment:

• OS: Windows
  • WPF

[thoemmi]

IMHO proper event handling is the responsibility of the consumer of the event.

• The library does not know anything about the application it is consumed by. It could be a desktop application like in your case, but also it could be a Windows service. Therefore the library cannot make any assumption about the used threading model, hence must be implemented on the consumer's side.
• A library must protect itself against misbehaving event handlers. Even if a handler throws an exception, the library must be able to continue its work. Esp if the error is transient.

Just my $0.02 😇

[roddone]

I agree with @thoemmi , his answer is pretty much what i would have answered myself. As he pointed out, the fact that the event is not executed in the UI context is not the responsibility of the library, it's the job of the consumer. If it does not suits your needs (or the StackOverflow guy needs), DeviceLocator.Discover() is an async methods that returns the list of the devices, you can use it directly in your UI thread instead of the OnDeviceFound event.

Furthermore, the empty catch statement is here to prevent errors during tasks creation (because of networkInterface issues for example), it does not "swallow" exceptions that happens inside the handlers. In the test program, throw an exception inside OnDeviceFound, you'll see it is catched in Program.cs's catch, not in DeviceLocator.cs.

Does not seems to be the "not trustworthy reliable library" you described on SO ...

[thoemmi]

@BionicCode, I don't agree on what you're saying about background threads. It's never guaranteed that event handlers are called synchronously. For sure it's valid for events raised by UI components, but for other events, especially if they rely on I/O, the consumer must expect that they're raised in a different thread.

However, IMHO you're right that exceptions should be swallowed. This might lead to undiscovered bugs. Either Discover should be more robust, so that it only catches exceptions when it performs it's discovery (and even then only IOExceptions), or should provide an OnError event, which it could use like this:

if(devices.TryAdd(device.Hostname, device))
{
    try
    {
        OnDeviceFound?.Invoke(null, new DeviceFoundEventArgs(device));
    }
    catch (Exception ex)
    {
        OnError?.Invoke(null, new ErrorEventArgs(ex));
    }
}

This wouldn't be the cleanest solution, but very more easy to implement, and at least the consumer would have a chance to get information about the exception.

Ideally, YeelightAPI would provide an IAsyncEnumerable<Device> DiscoverAsync() :wink:

[roddone]

I still disagree with @BionicCode about the background thread case, but you are both right about the events and the lack of robustness of this method. To be honnest I didn't spent a lot of time on it, i mostly focused on the protocol implementation and didn't really took care of the device discovery since it worked well until now 🙄

Unfortunately, I don't have time to work on this for now, so if you want to propose a solution, feel free to PR it 😉 otherwise i will do that later when I will got some free time.

There is still #25 to be taken in consideration and that could be related.

[roddone]

Hi there, why does all @BionicCode messages disappeared ? There was relevant ideas in them ...

Anyway, I would love to use IAsyncEnumerable<Device> DiscoverAsync() as @thoemmi mentioned, but sadly C#8 is not available For .Net Framework 🙁 Maybe we can implement this overload only for .NetStandard2.1 and higher with compiler directives, somethig like : #if !NETSTANDARD2_0 && !NET45 && !NET46 && !NET47.

Otherwise, I read all your propositions and I think the best solution is to keep (or maybe deprecate) the current Discover() method and create another one named "DiscoverAsync" which will get rid of the event that is the cause of this issue. DiscoverAsync() will accept a callback (System.Progress) that won't be present in the NetStandard2.1 overload. It should also fix the thread-context issue.

Are you OK with this ? Do you see anything to add ?

[thoemmi]

I cannot see @BionicCode's comments anymore too :thinking:

DiscoverAsync() will accept a callback (System.Progress) that won't be present in the NetStandard2.1 overload. It should also fix the thread-context issue.

How would System.Progress circumvent the thread-context issue?

@BionicCode wrote in his last, now vanished, comment:

2. Currently the event handler for the event OnDeviceFound are executed concurrently, which introduces possible serious problems to the client code, if the handler accesses shared resources (caller never knows about concurrency and that resources are actually shared), but is not thread-safe (which certainly never is the case, unless the developer checks implementation details) Proposed solution: use Dispatcher to to delegate to UI thread which would synchronizes handler invocations as they will be enqueued to the dispatcher queue

I hope you don't consider introducing a dependency on any UI library, such as WinForns or WPF. In fact, I use your library in a Windows Service, and I don't have any thread issues.

[roddone]

Don't worry I don't plan to do such a thing ;) (I use this library from a Windows Service too.) That's why I want to use the Progress<T> class, because according to the MSDN documentation :

Any handler provided to the constructor or event handlers registered with the ProgressChanged event are invoked through a SynchronizationContext instance captured when the instance is constructed. If there is no current SynchronizationContext at the time of construction, the callbacks will be invoked on the ThreadPool.

From my understanding, if the Progress<T> instance is created from an UI thread (WPF or Winform), the callbacks will use the Synchronization context matching the UI thread, otherwise it will keep the same behavior as today. Or maybe I'm wrong ?

[BionicCode]

Hi @roddone, thanks for your reply. I deleted my previous post myself, as I got the feeling that I went too far and may have annoyed both of you. Maybe I was wrong. But I apologize for criticizing too much though. But this wasn't arrogance, I can assure you :flushed: Just passion. I love my business and that's why I wanted to improve something, even if it doesn't belong to me. I mean for some reason I came by to open this ticket. Absolutely no bad intention. I really like your project. Lots of people find it useful and use it. You guys obviously did a good job.

C# 8 has some nice features. But I've read that they won't be all available in future .Net Framework releases as they would need to require runtime changes, which would break backward compatibility. We should always try to target at least .Net Standard 2.1+ for future projects in order to get the latest API and language features, but only if we don't have to remain compatible with the .Net Framework.

I personally think that your project is destined to run in a .Net Core runtime or Mono if you like to be available e.g. on Xamarin platforms too. If you don't depend on .Net Framework, you should do the step. But I wouldn't mix code by introducing compiler directives to switch execution paths. I would do a clean separation and introduce new assemblies, where each targets a different runtime or platform. Generally, if I want to support .Net Framework and .Net Core I would create three assemblies. The first accumulates standardized cross-platform features e.g. by targeting .Net Standard 2.0. This will be the dependency for all other platforms as they will build upon it. Then the other two will target .Net Framework and .Net Core. They may differ in features and implementation details. I don't know how realistic this is for you, as it will introduce some extra effort to maintain the library. But it would lever usability and flexibility as users could now run it on Linux machines too. So the user can decide which package he would like to install to target a specific platform. But it's a matter of taste and just my opinion. I think Microsoft simplest recommendation on how to target multiple platforms is exactly the way you've described using preprocessor directives.

My private opinion is you should completely remove the event-driven API. I really like the idea of using Progress<T>. @thoemmi It's perfect, because it internally marshals the callback execution back to the caller's thread. The caller must create an instance of Progress<T>. The constructor captures the current SynchroinizationContext. Whenever and on whatever thread the registered callback is invoked, it will execute on this very captured context. You were absolutely right not to introduce a dependency on a specific UI platform (what Dispatcher apparently would do). Application could run in a console application or as a service, indeed. Progress<T> will solve this. I mean you could simply implement the context capturing yourself, but why worry. Very good idea :+1: I like :smiley:

[BionicCode]

@roddone @thoemmi I think I would implement two overloads of DeviceLocator.DiscoverAsync: DiscoverAsync() : Task<IEnumerable<Device>> and DiscoverAsync(IProgress<T>) : Task<IEnumerable<Device>>. Then either a third and fourth overload with the same signature except they would return IAsyncEnumerable<Device> or copy and convert them to a dedicated .Net Core 3.1 assembly. Just let me know if I can do something.

[BionicCode]

@roddone @thoemmi I've added a PR. I just saw that there is a pending one (#25 ), which has changes in the same context and needs to be merged carefully. I didn't touched the target framework and therefore didn't implemented the solution which uses the new IAsyncEnumerable. If you agree, I would introduce a new assembly which targets .NET Standard 2.1 and implement it there.

[BionicCode]

@roddone Since it is my fault that I didn't pay attention to #25, just let me know when you've merged this request. I'll then pull and merge those changes locally and add a new PR. I refactored a lot, so merging could be complicated.

[BionicCode]

@roddone One more (last) question regarding the current implementation of DeviceLocator.CreateDiscoverTasks, line 117: why do you repeat the whole lookup three times? Is this intended? This outer loop has no breakout criteria and will repeat the perhaps already successful discovery. Removal (or at least adding a breakout criteria) should significantly improve performance. I have removed this outer loop, as it doesn't appear reasonable to me. But I can put it back, if this is intentional.

[roddone]

@roddone @thoemmi I've added a PR. I just saw that there is a pending one (#25 ), which has changes in the same context and needs to be merged carefully. I didn't touched the target framework and therefore didn't implemented the solution which uses the new IAsyncEnumerable. If you agree, I would introduce a new assembly which targets .NET Standard 2.1 and implement it there.

Thank you for your work with the refactoring and improvements on DeviceLocator. I need time to decide what to do with .Net Standard 2.1 and if you don't mind I would deal with it in a second time. I've created a new ticket to deal with this : #30 .

@roddone Since it is my fault that I didn't pay attention to #25, just let me know when you've merged this request. I'll then pull and merge those changes locally and add a new PR. I refactored a lot, so merging could be complicated.

Do not pay attention to #25 for now, this is a problem I still have to deal with. The problem i'm facing is that I can't reproduce it so I don't want to merge this PR for now, I will do it when I will have a full understanding of the problem and its resolution. I will merge it myself later ;)

@roddone One more (last) question regarding the current implementation of DeviceLocator.CreateDiscoverTasks, line 117: why do you repeat the whole lookup three times? Is this intended? This outer loop has no breakout criteria and will repeat the perhaps already successful discovery. Removal (or at least adding a breakout criteria) should significantly improve performance. I have removed this outer loop, as it doesn't appear reasonable to me. But I can put it back, if this is intentional.

In some rare cases or with slow networks no response was received from the devices with a single try. So this was an intentional retry process to maximize the chances to find devices. Maybe the retry count can be a static parameter like DeviceLocator.TryCount = (int) ?

[BionicCode]

Hi @roddone. Thanx for your reply. I put back the retry loop and added a public static RetryCount : int property.

[roddone]

Hi there, just a word to tell you that I do not forget this issue, but as you should know situation in France is a little bit "complicated" for the moment because of COVID19... There are a lot of things that requires my attention (at work and at home), so I will focus on YeelightAPI when things will return to normal 😉 See you soon!

[BionicCode]

Hi! Don't worry. I really hope that we all stay healthy and don't experience any complications. All the best for you and your families. And take care for your neighbors too. Hope to see you very soon :smiley: