Skip to content

feat: context managers in ServiceBrowser and AsyncServiceBrowser#1233

Merged
bdraco merged 2 commits into
python-zeroconf:masterfrom
Tenebrosus3141:master
Aug 27, 2023
Merged

feat: context managers in ServiceBrowser and AsyncServiceBrowser#1233
bdraco merged 2 commits into
python-zeroconf:masterfrom
Tenebrosus3141:master

Conversation

@Tenebrosus3141

@Tenebrosus3141 Tenebrosus3141 commented Aug 24, 2023

Copy link
Copy Markdown
Contributor

I started using this package for the first time recently, and when trying to use the async browser example, I found the requirement to use AsyncServiceBrowser.async_cancel() at the end of everything. After discovering that AsyncZeroconf has an async context manager, I found that AsyncServiceBrowser did not have its own.

I tried my best to replicate #284 (#177), however I am not sure how to go about adding test coverage. Apologies for not adding any.

Here's an example of how my code is simplified using the context manager (in Python 3.11):

async def main():
    services = ["_example._tcp.local."]
    async with (
        AsyncZeroconf() as aiozeroconf,
        AsyncServiceBrowser(aiozeroconf.zeroconf, services, handlers=[
            lambda **kwargs: tg.create_task(async_display_service_info(**kwargs)) and None
        ]),
        asyncio.TaskGroup() as tg,
    ):
        await asyncio.sleep(5)


if __name__ == '__main__':
    asyncio.run(main())

@bdraco

bdraco commented Aug 26, 2023

Copy link
Copy Markdown
Member

Can you add a test for these?

Also generally its not recommended to have short running instances if you can avoid it since it churns the network traffic quite a bit. There are obviously uses cases for it, and people do get it wrong a lot (and leak browsers) so I'm inclined to move this forward

@bdraco bdraco changed the title Support for context managers in ServiceBrowser and AsyncServiceBrowser feat: context managers in ServiceBrowser and AsyncServiceBrowser Aug 26, 2023
@Tenebrosus3141 Tenebrosus3141 force-pushed the master branch 4 times, most recently from cb069ef to 048d856 Compare August 27, 2023 01:33
@codecov

codecov Bot commented Aug 27, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.04% ⚠️

Comparison is base (dd637fb) 99.82% compared to head (c87af16) 99.78%.
Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1233      +/-   ##
==========================================
- Coverage   99.82%   99.78%   -0.04%     
==========================================
  Files          25       25              
  Lines        2798     2809      +11     
  Branches      480      480              
==========================================
+ Hits         2793     2803      +10     
  Misses          3        3              
- Partials        2        3       +1
Files Changed Coverage Δ
src/zeroconf/__init__.py 100.00% <100.00%> (ø)
src/zeroconf/_services/browser.py 99.17% <100.00%> (+0.02%) ⬆️
src/zeroconf/asyncio.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Tenebrosus3141

Tenebrosus3141 commented Aug 27, 2023

Copy link
Copy Markdown
Contributor Author

Can you add a test for these?

I added tests now

Though there seems to be a race condition between calling ServiceBrowser.cancel() or AsyncServiceBrowser.async_cancel() and checking the value of _ServiceBrowserBase.done, but I believe that may be out of scope for this PR

Also generally its not recommended to have short running instances if you can avoid it since it churns the network traffic quite a bit. There are obviously uses cases for it, and people do get it wrong a lot (and leak browsers) so I'm inclined to move this forward

My use case may be for that, my own project intends to just run the browser briefly at startup

@bdraco

bdraco commented Aug 27, 2023

Copy link
Copy Markdown
Member

Async_cancel should not race

Cancel should but you can do asyncio.sleep(0) to ensure the call soon is run

@Tenebrosus3141

Tenebrosus3141 commented Aug 27, 2023

Copy link
Copy Markdown
Contributor Author

I put the call to asyncio.sleep(0) in the synchronous browser test and removed it from the async one now
It should be ready now

bdraco
bdraco reviewed Aug 27, 2023
View reviewed changes
Comment thread tests/services/test_browser.py Outdated
bdraco
bdraco reviewed Aug 27, 2023
View reviewed changes
Comment thread tests/services/test_browser.py Outdated
@Tenebrosus3141

Tenebrosus3141 commented Aug 27, 2023

Copy link
Copy Markdown
Contributor Author

Maybe use asyncio.run_coroutine_threadsafe(asyncio.sleep(0), zc.loop).result() instead to keep the test sync so the sync versions are used in the expected way

That makes more sense, I wasn't sure how to do that until you pointed out how, thanks

bdraco
bdraco approved these changes Aug 27, 2023
View reviewed changes

@bdraco bdraco left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @Tenebrosus3141

@bdraco bdraco merged commit bd8d846 into python-zeroconf:master Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bdraco bdraco bdraco approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Tenebrosus3141 @bdraco