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

fix #60 #61

Merged
merged 6 commits into from
Feb 22, 2019
Merged

fix #60 #61

merged 6 commits into from
Feb 22, 2019

Conversation

hubo1016
Copy link
Contributor

@hubo1016 hubo1016 commented Feb 15, 2019

This PR adds async context manager support on Watch object, so the response is correctly closed when watch is stopped with stop() or by some exceptions.

fix #60

@codecov-io
Copy link

codecov-io commented Feb 15, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@d57e9e9). Click here to learn what that means.
The diff coverage is 96.77%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #61   +/-   ##
=========================================
  Coverage          ?   94.46%           
=========================================
  Files             ?       20           
  Lines             ?     1373           
  Branches          ?        0           
=========================================
  Hits              ?     1297           
  Misses            ?       76           
  Partials          ?        0
Impacted Files Coverage Δ
kubernetes_asyncio/watch/watch.py 94.25% <100%> (ø)
kubernetes_asyncio/watch/watch_test.py 95.95% <93.75%> (ø)

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 d57e9e9...aa1d3a2. Read the comment docs.

Copy link
Owner

@tomplus tomplus left a comment

Choose a reason for hiding this comment

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

I'm not sure if the context manager is necessary to fix the issue. What do you think about handling an exception in an iterator and close response object?

    async def __anext__(self):
        try:
            return await self.next()
        except:
            self.resp.close()
            self.resp = None
            raise

kubernetes_asyncio/watch/watch.py Outdated Show resolved Hide resolved
@hubo1016
Copy link
Contributor Author

@tomplus The except clause solves the problem partially, but the response may still leak if an exception is raised in the async for structure. In this case, the __anext__ function is not called. In general, most async generator interfaces need a explicit close operation, either with aclose() or __aexit__. Usually async with is easier to use.

@tomplus
Copy link
Owner

tomplus commented Feb 18, 2019

Yes, it makes sense, thanks for working on this!

examples/example2.py Outdated Show resolved Hide resolved
kubernetes_asyncio/watch/watch.py Outdated Show resolved Hide resolved
kubernetes_asyncio/watch/watch_test.py Show resolved Hide resolved
hubo added 2 commits February 20, 2019 11:02
- test `release()` called
@tomplus tomplus merged commit f83dc57 into tomplus:master Feb 22, 2019
@tomplus
Copy link
Owner

tomplus commented Feb 22, 2019

Released in v8.1.0, thank you.

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.

Calling watch.stop() does not close the response correctly
3 participants