search

michielpost / Q42.HueApi

C# helper library to talk to the Philips Hue bridge
MIT License
411 stars 114 forks source link

Entertainment thread safety/API issue #209

Open ermau opened 4 years ago

ermau commented 4 years ago

EntertainmentLayer inherits from List<EntertainmentLight>, exposing direct access to its own storage. However, within AutoCalculateEffectUpdate, it delegates work to another thread through Task.Run and reads through the same storage that it exposes without any locking mechanism. It does the same thing with Effects. If you call AutoCalculateEffectUpdate and later add an effect or remove a light, you may crash if the started task is reading at the same time you're writing.

Normally you could just add some locking around this, but since the storage mechanism is exposed, the user has no way to participate. I would have just submitted a PR, but I think the problem necessitates a breaking change to the API. I've done that in the past, but that was far earlier in the library's life so I wanted to start a discussion first as this same problem also occurs in other APIs.

I'd recommend avoiding inheriting directly from concrete collections for public API types in general, it locks you in to the type or forces you to make a breaking change if you have to change things later.

michielpost commented 4 years ago

Thanks for the suggestions. Certainly valid points and I've come across these threading issues myself in the past while working on HueLightDJ.

So if I understand correctly, you're suggestion is to not expose the Effects property, but require the usage of methods like public void PlaceEffect(BaseEffect baseEffect). And EntertainmentLayer should not inherit from List<EntertainmentLight> but keep that storage in a private property.

If you're willing to submit a PR, that would be great. I have no problem with a breaking change if it's for the better. The Entertainment API is not as widely used as the default HTTP api, so the impact won't be that big I think.

ermau commented 4 years ago

So if I understand correctly, you're suggestion is to [..]

Yeah at first glance, basically.

If you're willing to submit a PR, that would be great. I have no problem with a breaking change if it's for the better.

I'll go ahead and start on a PR.

I'm not familiar enough with the API yet to know what is need or isn't, so I'll have to pop back in with questions as I go. For example, is it a purposeful decision to allow the user to add and remove lights to a StreamingGroup? How about for an EntertainmentLayer?