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

Split read/write/connect timeout error classes #849

Open
maxim opened this issue Apr 19, 2024 · 5 comments
Open

Split read/write/connect timeout error classes #849

maxim opened this issue Apr 19, 2024 · 5 comments

Comments

@maxim
Copy link

maxim commented Apr 19, 2024

What do you think about splitting Timeout error classes into ReadTimeout/WriteTimeout/ConnectTimeout?

If I understand correctly, when handling destructive (non-idempotent) actions in APIs

  • a connect timeout means the action was definitely not performed
  • a write or read timeout means that the action may have been performed

So it seems different recovery procedures should take place to resolve the status of the action, depending on the type of the timeout.

Right now, we'd have to parse the error message to tell them apart.

Do you think it makes sense to have them all be their own classes, inheriting from Timeout? The inheritance would maintain backwards compatibility.

@geemus
Copy link
Contributor

geemus commented Apr 19, 2024

Hey, thanks for the detailed report. At least initially I would say that split makes sense to me. As you suggest, if they inherit it should nicely allow for backwards compatibility.

I also would agree that my expectation is that connect timeouts can safely be retried, whereas read/write timeouts are more ambiguous (you might or might not be able to make further assumptions depending on how much you know about the server in question).

So overall I am supportive of exploring this. It seems pretty straightforward so far, though perhaps we'll find challenges as we get into it. I'm certainly happy to support work on this, is this something you would be able and willing to work on? Thanks!

@maxim
Copy link
Author

maxim commented Apr 21, 2024

@geemus Great! I gave it a shot. Left a few notes in the PR, let me know if anything doesn't sit right with you.

@maxim
Copy link
Author

maxim commented Apr 21, 2024

Pushed a couple of fixups, hope I didn't mess up your review. 🤞🏻

@geemus
Copy link
Contributor

geemus commented Apr 23, 2024

@maxim no worries, I hadn't quite found time for review yet anyway. Will take a look now.

@maxim
Copy link
Author

maxim commented Apr 25, 2024

I will proceed after we make this decision, but absolutely no rush. Just clarifying why I'm not submitting revisions yet.

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

No branches or pull requests

2 participants