Skip to content
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

Caput with callback #98

Merged
merged 9 commits into from
Jul 29, 2022
Merged

Caput with callback #98

merged 9 commits into from
Jul 29, 2022

Conversation

AlexanderWells-diamond
Copy link
Collaborator

@AlexanderWells-diamond AlexanderWells-diamond commented Jul 28, 2022

Add blocking to the processing of on_update/on_update_name callbacks, to allow caput to correctly wait for processing to complete before returning.

Documented the new blocking flag on OUT record creation, and the SetBlocking function. I didn't document the CothreadDispatcher, as there's no reason for any users to explicitly create it themselves. Let me know if I should document it, or perhaps change its name to indicate its internal-only.

Tests written:

  • Various tests that the record-level and global blocking flag are correctly passed to the ProcessDeviceSupportOut class
  • A blocking record will correctly process blocking caput requests from both a single thread multiple times and from multiple threads simultaneously.

@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #98 (877a89c) into master (06d6b96) will increase coverage by 0.08%.
The diff coverage is 91.89%.

@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
+ Coverage   87.63%   87.72%   +0.08%     
==========================================
  Files          13       14       +1     
  Lines         930      961      +31     
==========================================
+ Hits          815      843      +28     
- Misses        115      118       +3     
Impacted Files Coverage Δ
softioc/pythonSoftIoc.py 90.00% <ø> (ø)
softioc/cothread_dispatcher.py 80.00% <80.00%> (ø)
softioc/device.py 98.40% <93.75%> (-0.33%) ⬇️
softioc/asyncio_dispatcher.py 93.33% <100.00%> (+0.47%) ⬆️
softioc/builder.py 97.41% <100.00%> (+0.01%) ⬆️
softioc/imports.py 100.00% <100.00%> (ø)
softioc/softioc.py 77.57% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06d6b96...877a89c. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jul 28, 2022

Unit Test Results

     15 files  ±  0       15 suites  ±0   21m 39s ⏱️ + 1m 29s
   230 tests +  5     224 ✔️ +  5      6 💤 ±0  0 ±0 
3 450 runs  +75  3 010 ✔️ +70  440 💤 +5  0 ±0 

Results for commit 877a89c. ± Comparison against base commit 06d6b96.

♻️ This comment has been updated with latest results.

Includes a "blocking" flag on record creation.

This change allows you to use caput in asynchronous mode, where it will
wait for record processing to complete.
This implementation still leaks abstractions - the device now needs to
know about the difference between cothread and async calls.
Perhaps another round of refactoring is required...
Also add single and multi threaded tests for blocking records.
Renamed the function to conform to naming convention.
Added CHANGELOG entry.
This leaks even more async code into the device, but I can't see a way
around it without having to require the dispatcher be provided before
creating records...
By creating/modifying the dispatchers we can easily handle the
__completion being called after the __on_update.
Without doing this there seem to be unavoidable implementation leaks,
where the device.py file would have to care about the difference
between cothread and asyncio.
Copy link
Collaborator

@Araneidae Araneidae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Have a couple of coding changes to suggest, particularly the asyncio dispatcher, but I think this is a huge success.

After all the pain this has come out a lot more cleanly than I was expecting!

softioc/device.py Outdated Show resolved Hide resolved
softioc/device.py Outdated Show resolved Hide resolved
softioc/device.py Outdated Show resolved Hide resolved
softioc/imports.py Outdated Show resolved Hide resolved
softioc/asyncio_dispatcher.py Outdated Show resolved Hide resolved
- SetBlocking returns the old blocking state value. Added test.
- Fix some style issues
- Make completion function on the dispatcher optional, to maintain some
compatibility with the previous API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants