Skip to content

Fix float("inf") timeouts - #846

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

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

Conversation

@karpetrosyan

@karpetrosyankarpetrosyan commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

Resolves #845

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

Simple demonstration:

Works

importasynciofromhttpcore._synchronizationimportAsyncEventasyncdefbackground(event: AsyncEvent):
awaitasyncio.sleep(2)
event.set()
asyncdefmain():
event=AsyncEvent()
asyncio.create_task(background(event))
awaitevent.wait(timeout=float("inf"))
print('done')
asyncio.run(main())

Doesn't work

importthreadingimporttimefromhttpcore._synchronizationimportEventdefbackground(event: Event):
time.sleep(2) event.set()
defmain():
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

@karpetrosyankarpetrosyan added the bug Something isn't working label Nov 7, 2023
@lovelydinosaur

Copy link
Copy Markdown
Contributor

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

karpetrosyan commented Nov 8, 2023

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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.

@lovelydinosaurlovelydinosaur left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
ContributorAuthor

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

Yes I would

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

Labels

bugSomething isn't working

Development

Successfully merging this pull request may close these issues.

2 participants

@karpetrosyan@lovelydinosaur