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

Throw new InterruptedIOException subtype on timeout #697

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dmgd
Copy link

@dmgd dmgd commented Feb 14, 2020

Introduces TimeoutException which a caller can catch if it just wants to just deal with timeout and deadline reached, but not interrupts.

Introduces TimeoutException which a caller can catch if it just wants to just deal with timeout and deadline reached, but not interrupts.
@dmgd
Copy link
Author

dmgd commented Feb 14, 2020

The specific case I'm dealing with is something that wants to transform timeouts into an http response, but leave interrupts alone. It could do this by catching InterruptedIOException and then working out whether it's a timeout or not + rethrowing if it's not, but this change would make the handling code much cleaner

@swankjesse
Copy link
Collaborator

Unfortunately you’re competing with SocketTimeoutException which also attempts to signal whether the failure was caused by timeout.

Introducing our own timeout exception feels like a complicated feature that we’d need to do everywhere, including significant tests in Okio and OkHttp.

@dmgd
Copy link
Author

dmgd commented Feb 14, 2020

Unfortunately you’re competing with SocketTimeoutException which also attempts to signal whether the failure was caused by timeout.

Introducing our own timeout exception feels like a complicated feature that we’d need to do everywhere, including significant tests in Okio and OkHttp.

Good point about SocketTimeoutException. We already catch and convert that, so reusing that would work perfectly for us. I didn't reuse it just because these timeouts are not really socket related, so it felt a bit wrong . . .

In terms of backwards compatibility, TimeoutException is still an InterruptedIOException and has the same message it already had, so the only thing that I can think of that could break would be comparisons using getClass() rather than instanceof

@swankjesse
Copy link
Collaborator

I definitely like subtyping InterruptedIOException. The holdup is more about finding all of hte places where we should be throwing TimeoutException.

@dmgd
Copy link
Author

dmgd commented Feb 15, 2020

So I think what I've just pushed might be everywhere that's relevant in okio (based on searches for "timeout" "deadline reached" and InterruptedIOException.) Is it the sort of thing you had in mind?

I'm taking a look at OkHttp now :)

@dmgd
Copy link
Author

dmgd commented Feb 17, 2020

dmgd/okhttp@71fa6b5
and
dmgd/okhttp@a9fb839
look like the okhttp changes that would go with this

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.

2 participants