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

Add timeout option to package:cronet_http #1186

Closed
techouse opened this issue Apr 30, 2024 · 6 comments
Closed

Add timeout option to package:cronet_http #1186

techouse opened this issue Apr 30, 2024 · 6 comments
Assignees
Labels
package:cronet_http type-enhancement A request for a change that isn't a bug

Comments

@techouse
Copy link

techouse commented Apr 30, 2024

Hi,

Are there any plans to add a timeout option to package:cronet_http?

I'm not sure if a timeout option was omitted by design or by mistake.

The source code seems to say it's disabled by design

@Override
public void setConnectTimeout(int timeout) {
    // Per-request connect timeout is not supported because of late binding.
    // Sockets are assigned to requests according to request priorities
    // when sockets are connected. This requires requests with the same host,
    // domain and port to have same timeout.
    Log.d(TAG, "setConnectTimeout is not supported by CronetHttpURLConnection");
}
@techouse techouse added package:cronet_http type-enhancement A request for a change that isn't a bug labels Apr 30, 2024
@brianquinlan
Copy link
Collaborator

Can you use the timeout method on the future returned by the client?

@techouse
Copy link
Author

techouse commented May 1, 2024

Well, I could, however, that kinda breaks the "plug & play" rationale as it involves additional application logic outside the Client. I was hoping for a timeout error to be thrown from the send method

Future<StreamedResponse> send(BaseRequest request) async {

I could make a PR if that's the route this should take. Otherwise I'll simply defer migrating from IOClient to Cronet on Android.

@brianquinlan
Copy link
Collaborator

@techouse I don't think that we should add timeout capabilities for a single client.

See #21

@techouse
Copy link
Author

techouse commented May 2, 2024

@techouse I don't think that we should add timeout capabilities for a single client.

@brianquinlan Hmm, I'm not sure I follow as both cupertino_http and HttpClient have a timeout option, just cronet_http is missing it.

I've taken your initial suggestion of adding a Future.timeout lejard-h/chopper#604

@brianquinlan
Copy link
Collaborator

Hey @techouse ,

I thought that you were looking for a per-request timeout.

timeoutIntervalForRequest can be used to configure the timeout for all requests made by the Client
connectionTimeout is per-Client and it also only applies to the connection phase of the request - the response can take an arbitrary amount of time.

@techouse techouse closed this as completed May 2, 2024
@absar
Copy link

absar commented Nov 9, 2024

Hi @brianquinlan ,
Does it mean there is no cronet equivalent of cupertino_http.timeoutIntervalForRequest?
Adding a timeout to client future does not seem appropriate, since it's not going to cancel the request after timeout, secondly the timeout on future will essentially be for the whole response duration e.g. client.get as opposed to just request/connection timeout.

@techouse i think your original request is still valid, this ticket should not be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:cronet_http type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants