Skip to content

Fix float("inf") timeouts#846

Merged
lovelydinosaur merged 2 commits intoencode:masterfrom
karpetrosyan:fix-timeouts
Nov 8, 2023
Merged

Fix float("inf") timeouts#846
lovelydinosaur merged 2 commits intoencode:masterfrom
karpetrosyan:fix-timeouts

Conversation

@karpetrosyan
Copy link
Contributor

@karpetrosyan karpetrosyan commented Nov 7, 2023 •
edited
Loading

Resolves #845

The issue is that our AsyncEvent.wait supports float('inf') timeout but Event.wait doesn't.

Simple demonstration:

Works

import asyncio
from httpcore._synchronization import AsyncEvent

async def background(event: AsyncEvent):
    await asyncio.sleep(2)
    event.set()

async def main():
    event = AsyncEvent()
    asyncio.create_task(background(event))
    await event.wait(timeout=float("inf"))
    print('done')

asyncio.run(main())

Doesn't work

import threading
import time
from httpcore._synchronization import Event

def background(event: Event):
    time.sleep(2) 
    event.set()

def main():
    event = Event()
    threading.Thread(target=background, args=[event]).start()
    event.wait(timeout=float("inf"))
    print('done')

main()

Error:

Traceback (most recent call last):
  File "/home/test/programs/gitprojects/httpcore/tust.py", line 15, in <module>
    main()
  File "/home/test/programs/gitprojects/httpcore/tust.py", line 12, in main
    event.wait(timeout=float("inf"))
  File "/home/test/programs/gitprojects/httpcore/httpcore/_synchronization.py", line 229, in wait
    if not self._event.wait(timeout=timeout):
  File "/usr/lib/python3.10/threading.py", line 607, in wait
    signaled = self._cond.wait(timeout)
  File "/usr/lib/python3.10/threading.py", line 324, in wait
    gotit = waiter.acquire(True, timeout)
OverflowError: timestamp too large to convert to C _PyTime_t

@karpetrosyan karpetrosyan added the bug Something isn't working label Nov 7, 2023
@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented Nov 8, 2023

How would you feel about us starting by just reverting #823 for a 1.0.2 release?

I feel like I'd like a slower review onto #822 / #823 (which isn't critical).

@karpetrosyan
Copy link
Contributor Author

karpetrosyan commented Nov 8, 2023 •
edited
Loading

How would you feel about us starting by just reverting #823 for a 1.0.2 release?

This means that we would not support pool timeout, which I believe is critical.

Also, look at #814. The previous behavior was very unstable and unpredictable.
#822 has an example approving that we do not support pool timeouts.

@lovelydinosaur
Copy link
Contributor

lovelydinosaur commented Nov 8, 2023

So...

We have a stable version with a minor issue that some timeouts can take longer than expected in some circumstances.
We've "upgraded" that to a more complex version that introduces an overflow error.

My strong preference in a case like that is to immediately roll back the change, re-release, and then make sure we're slowing down on future changes.

I'm up for re-reviewing #822 and #823 (plus this change) but I'd much rather be able to do that in a way that's not time-pressured.

@karpetrosyan
Copy link
Contributor Author

karpetrosyan commented Nov 8, 2023

It's up to you.

My strong preference in a case like that is to immediately roll back the change, re-release, and then make sure we're slowing down on future changes.

I would agree if the issue was not small enough to be fixed immediately in the next release.

lovelydinosaur
lovelydinosaur approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@lovelydinosaur lovelydinosaur left a comment

Choose a reason for hiding this comment

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

👍 Okay, yep. You've probs got a good call on this one.

Would you also like to roll up a 1.0.2 release for this?

(thanks)

@karpetrosyan
Copy link
Contributor Author

karpetrosyan commented Nov 8, 2023

Would you also like to roll up a 1.0.2 release for this?

Yes I would

@lovelydinosaur lovelydinosaur merged commit 1e7597b into encode:master Nov 8, 2023
@karpetrosyan karpetrosyan mentioned this pull request Nov 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lovelydinosaur lovelydinosaur lovelydinosaur approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@karpetrosyan @lovelydinosaur