search

Mr-Markus / ZigbeeNet

A .NET Standard library for working with ZigBee
Eclipse Public License 1.0
131 stars 47 forks source link

Remove unused members; minor refactoring; don't lock on pulic members… …; adjust locking #101

Closed nicolaiw closed 4 years ago

spudwebb commented 4 years ago

@nicolaiw next time could you rebase your branch on develop head before issuing the pull request. I find it hard to read the commit history of the develop branch when there is a lot of merge commits.

spudwebb commented 4 years ago

why do we need a lock here? https://github.com/Mr-Markus/ZigbeeNet/blob/778993d4e754874043161b569bc163f7fa0b1072/libraries/ZigBeeNet/ZigBeeNode.cs#L331-L334 I don't see how locking a single return statement can have any use

I don't understand that one either: https://github.com/Mr-Markus/ZigbeeNet/blob/778993d4e754874043161b569bc163f7fa0b1072/libraries/ZigBeeNet/ZigBeeNode.cs#L468-L471

nicolaiw commented 4 years ago

I will have a look at it .. I guess the look is needed .. but it might by wrong .. just imagin a sceneario for the second case where if there was no lock ..

Thread1 (T1) evalutes the value returnd from TryGetValue() and access the if path .. now before the calling endpoint.ComandReceived() a context switch happend and T2 does something with exactly that endpoint ... something like assigning it to null .. now .. contextswitch again .. T1 now calls endpoint.CommandReveived() --> NullRefEx .. but yes .. I guess we have to lock the whole if statement in this case .. for the first case im not sure if a contextswitch could happen betwen the evaluation of the TryGetValue() return value and the inner return ..

EDIT: I guess we will not need the locks .. as the variables a local .. I will adjust the code,

spudwebb commented 4 years ago

endpoint is a local variable, how could it be assigned to null by another thread?

nicolaiw commented 4 years ago

See my EDIT :D