nrkno / sofie-emberplus-connection

Ember+ Connection: A Part of the Sofie TV Studio Automation System
https://github.com/nrkno/Sofie-TV-automation/
MIT License
20 stars 15 forks source link

fix: Setting 2 full path properties fails SOFIE-2628 #34

Closed Julusian closed 11 months ago

Julusian commented 11 months ago

I think what is happening is:

The change detection logic is pretty naive and isnt factoring in that getDirectory is expecting the children property to be loaded. So the first 'change' which is updating solely the contents is completing the request, and confusing the getElementByPath which is expecting the children object to have been loaded by the time that the getDirectory call resolves.


The change here introduces a simple enum, so that the getDirectory request can flag as needing the children to be loaded before it can be resolved. Other calls should behave the same as before, with the previous hasResponse being replaced with values in this enum

codecov-commenter commented 11 months ago

Codecov Report

Attention: 41 lines in your changes are missing coverage. Please review.

Comparison is base (7682c4e) 60.94% compared to head (e5698e5) 60.76%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #34 +/- ## ========================================== - Coverage 60.94% 60.76% -0.19% ========================================== Files 69 69 Lines 6058 6076 +18 Branches 555 555 ========================================== Hits 3692 3692 - Misses 2355 2373 +18 Partials 11 11 ``` | [Files](https://app.codecov.io/gh/nrkno/sofie-emberplus-connection/pull/34?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nrkno) | Coverage Δ | | |---|---|---| | [src/Ember/Client/index.ts](https://app.codecov.io/gh/nrkno/sofie-emberplus-connection/pull/34?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nrkno#diff-c3JjL0VtYmVyL0NsaWVudC9pbmRleC50cw==) | `0.00% <0.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Julusian commented 11 months ago

I can see this solution fixing the issue specifically for the getElementByPath case but in essence the race condition is not fundamentally fixed. Unfortunately, given how emberplus reports all its responses through updates to the tree it seems we would have to do very specific matching of how the tree is updated to whether that corresponds with the contents of the command to do a fundamental fix. (Or move the more execution synchronous model which would sadden me a bit)

Yes, I haven't attempted to fix the underlying race condition. I don't think I know enough about ember+ to be comfortable making that large of a change much here.
But I am not sure that the race condition is really an issue, and more importantly I dont know how it could be avoided.
Even if this library was more synchronous, there is still a risk of a subscription update during a request which could trip up the state handling logic. So without a way to match a request to a response, I think that this new approach of validating an update matches what a request asked for is the way to go

mint-dewit commented 11 months ago

I agree. Another optimisation would be to implement some logic in the getElementByPath to not create these duplicate requests but this PR is fine as is by me.