Cherry-Pick safe and tested commits from bled112-refactor into master
#933
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, when stop() was called on an async wrapper on top of a legacy device adapter, it would result in two calls to stop() which would stop the underlying serial device twice on the bled112 adapter. This commits fixes the issue by deferring all actual stop behavior to the background task that manages the legacy adapter and stopping that task when stop() is called directly.
There's a CPython bug that causes event loops running in background threads to raise an exception on close. The cause is that the default event loop changed to ProactorEventLoop on windows and there was a latent bug using a ProactorEventloop in a background thread. This commit checks for this issue and patches the event loop policy only on python 3.8.0 and windows to use the old SelectorEventloop.
OperationManager before was basically a POC of how you could create a class that lets you write coroutines to block on complex network activity without requiring one thread per blocking operation or complex callback based code. The tricky part of writing such coroutines is that you tend to get race conditions when performing complex operations such as "gather" operations where you send a command that is acknowledged and then results in a flurry of events terminated by a "finished" event. You need to be able to wait for the first ack event but then wait for an unknown number of events terminated by a known event. Without careful planning you have a race condition where you install a filter to look for events after you receive the acknowledgement packet but if the first event comes very soon after you'll miss it. The traditional fix is to have a single thread process packets one at a time with a single operation in flight to pause packet processing until you setup your next filters. OperationManager lets you do that with coroutines so you can have multiple parallel operations running without having a race in any of them.
timburke
changed the title
bled112-refactor into masterbled112-refactor into master
mattrunchey
approved these changes
Dec 20, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@mattrunchey These commits were already reviewed and approved in the
feat-bled112-refactorbranch last week (or the week before). I'm pulling them out and cherry-picking over to master because they're generally useful apart from that refactor and safe.I'm working to make
feat-bled112-refactorless massive so that we can get smaller PRs into master with backwards compatible improvements without a giant long running feature-branch merge at the end.The current plan is:
feat-bled112-refactorinto master (this PR) so that the feature branch becomes smaller.feat-bled112-refactorto use the code now iniotile-transport-blelib.