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.

Sign up for GitHub

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

Jump to bottom

Add retry on 504 Gateway Time-out and "connection closed before message completed" #198

Closed
Christof23 opened this issue Mar 5, 2024 · 5 comments
Closed

Add retry on 504 Gateway Time-out and "connection closed before message completed" #198

Christof23 opened this issue Mar 5, 2024 · 5 comments
Labels
external depedency no-op External depedencies requires work with upstream provider and not this crate

Comments

@Christof23
Copy link

Christof23 commented Mar 5, 2024 •
edited
Loading

Hi, I can see that requests are retried on a 429 too many requests status code. However on other error types there are no retries.

For example for 504 responses the response returns:

ERROR async_openai::error:46: failed deserialization of: <html>
<head><title>504 Gateway Time-out</title></head>
<body>
<center><h1>504 Gateway Time-out</h1></center>
<hr><center>openresty</center>
</body>
</html>

Similarly, I sometimes receive Error: IncompleteMessage: connection closed before message completed for some requests which are not retried. I believe this is a hyper issue: hyperium/hyper#2136
@Christof23
Copy link
Author

PR for the 504: #199

@64bit
Copy link
Owner

64bit commented Mar 6, 2024

Hi @Christof23 ,

Thank you for the issue and PR.

What rationale is there to retry when API is down - and could potentially be down for hours until incident is resolved?

@64bit
Copy link
Owner

64bit commented Mar 6, 2024

Similarly, I sometimes receive Error: IncompleteMessage: connection closed before message completed for some requests which are not retried. I believe this is a hyper issue: hyperium/hyper#2136

Based on the comment, does configuring same parameter for custom reqwestclient fix the issue?

@Christof23
Copy link
Author

Hi @64bit, thanks for your response.

I take your point on the 504 response; however I've seen this response while other concurrent requests have gone through successfully. I'm not sure how OpenAI are configuring their infrastructure, but it seems that a 504 with their API doesn't necessarily mean the API is unavailable.

For the reqwest client, I think I tried the fix with the client configuration, but I'll double check.

@64bit
Copy link
Owner

64bit commented Mar 6, 2024

Right thing to do here is contact OpenAI and report that their API is failing. Introducing retries is propagating that failure to everyone - and making library in-deterministic.

That said, one thing that could be done in async-openai is propgate the payload causing deserialization error so that library users can decide what right action to perform.

So OpenAIError::JSONDeserialize(serde_json::Error), becomes OpenAIError::JSONDeserialize(serde_json::Error, String), where the String is payload that OpenAI responded with.

@64bit 64bit added the external depedency no-op External depedencies requires work with upstream provider and not this crate label Mar 19, 2024
@64bit 64bit closed this as completed May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
external depedency no-op External depedencies requires work with upstream provider and not this crate
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@64bit @Christof23