-
Notifications
You must be signed in to change notification settings - Fork 5
Fix an issue with dropped sync callbacks #69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
=======================================
Coverage 99.34% 99.34%
=======================================
Files 4 4
Lines 455 457 +2
=======================================
+ Hits 452 454 +2
Misses 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a85450c to
fecc7f9
Compare
AlexanderWells-diamond
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks mostly fine, although I think there's a possible change of behaviour outside the scope of the description? The PR only discusses the initial condition of multiple updates before the first call to __create_subscription, and this now correctly handles sending all values that were received. However, it will now also do that for every subsequent update as well (as it'll loop around the while not self.__is_sync: loop). Is that intended? Do we just expect it to be fast enough once connected that there is no difference?
Another question I have is whether it is worth writing one/some unit tests for this; as this won't ever be tested in CI due to runner speed, and we can't guarantee everyone will run the tests locally before committing, a few unit tests for this seem like a good idea.
I convinced myself that the old and new behaviour would be the same for the async case (because both __signal and __create_subscription are running under the event loop), but we should discuss offline...
That probably makes sense, I'll try and make one |
537cc26 to
b51c17c
Compare
When doing camonitor with a sync callback, if the PV receives 2 or more updates between connect and the first callback firing, then only the first callback will be called. This only happens when all_updates=True because with all_updates=False they will be squashed into a single callback with the latest value. The issue is that __signal wakes up __create_subscription when there are already multiple pending_values, but __create_subscription handles the first, finds it is sync, then hands back to __signal, which is not called on the remaining values. This fixes it by making __create_subscription handle all the updates it finds on the initial pass
f3ccaad to
ebc7bf8
Compare
When doing camonitor with a sync callback, if the PV receives 2 or more updates between connect and the first callback firing, then only the first callback will be called. This only happens when all_updates=True because with all_updates=False they will be squashed into a single callback with the latest value.
The issue is that __signal wakes up __create_subscription when there are already multiple pending_values, but __create_subscription handles the first, finds it is sync, then hands back to __signal, which is not called on the remaining values. This fixes it by making __create_subscription handle all the updates it finds on the initial pass
Fixes #68
Fixes #62