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

Change behaviour for timeout <= 0 #28

Merged
merged 12 commits into from
Oct 9, 2017

Conversation

hellysmile
Copy link
Member

@hellysmile hellysmile commented Sep 23, 2017

fix for #27

and minor fix for raising RuntimeError when used outside task and timeout is None

@codecov
Copy link

codecov bot commented Sep 23, 2017

Codecov Report

Merging #28 into master will increase coverage by 1.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
+ Coverage   70.37%   71.42%   +1.05%     
==========================================
  Files           1        1              
  Lines          54       56       +2     
  Branches        9        9              
==========================================
+ Hits           38       40       +2     
  Misses         15       15              
  Partials        1        1
Impacted Files Coverage Δ
async_timeout/__init__.py 71.42% <100%> (+1.05%) ⬆️

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 a68e482...eb76fda. Read the comment docs.

@hellysmile
Copy link
Member Author

Locally tests on Python 3.6 works fine, can someone restart travis build for 3.6?

@fafhrd91
Copy link
Member

restarted, but it failed again

@hellysmile
Copy link
Member Author

I've had Python 3.6.1 locally and it works there, but fails on Python 3.6.2. I'll take a look on it

@hellysmile
Copy link
Member Author

run_until_complete was changed in python/cpython#1688 , it brings fails

@hellysmile
Copy link
Member Author

This cgange right now braks expired proerty , Ill fix it

@hellysmile
Copy link
Member Author

I think it is not possible to raise immediately asyncio.TimeoutError and support returning context manager instance.

Any design\concept ideas?

@hellysmile
Copy link
Member Author

only one way is

cm = timeout(0)
with cm:
    yield from asyncio.sleep(1)

assert cm.expired

@hellysmile
Copy link
Member Author

Probably we can document that using properties like expired or remaining need to be used as example above. As well it "gives" ability to reuse context manager, maybe makes sense to add check that it can be used only ones, or prevent multiple __enter__ in same time?

raise RuntimeError('Timeout context manager should be used '
'inside a task')

if self._timeout is None:
Copy link
Member

Choose a reason for hiding this comment

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

Move these lines before self._task check: it allows to run aiohttp client from tornado with disabled timeouts.

@Kentzo
Copy link
Contributor

Kentzo commented Sep 28, 2017

I support the idea of raising a timeout error ASAP when timeout is <=0.

@hellysmile
Copy link
Member Author

hellysmile commented Sep 28, 2017

@Kentzo it is possible to raise asyncio.TimeoutError even in __enter__ method, but in this case any context manager features like expired will be lost

I had conversation with @asvetlov offline regarding raising asyncio.TimeoutError instantly and decision was to support context manager, even there is some time for synchronous code inside with block.

It will be great to hear more feedback, what is more important support context manager or raise asyncio.TimeoutError

@Kentzo
Copy link
Contributor

Kentzo commented Sep 29, 2017

@hellysmile I'd agree with @asvetlov regarding not raising the exception instantly. My comment was regarding removing "0 = no timeout" semantics.

@hellysmile
Copy link
Member Author

Is there any concerns regarding this PR?

@asvetlov asvetlov merged commit 047fa06 into aio-libs:master Oct 9, 2017
@asvetlov
Copy link
Member

asvetlov commented Oct 9, 2017

Everything is perfect, thank you.

@hellysmile hellysmile deleted the change_timeout_0 branch October 9, 2017 21:55
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.

None yet

4 participants