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

Expose options for underlying http client #59

Open
jgehrcke opened this issue Nov 28, 2019 · 7 comments · May be fixed by #132
Open

Expose options for underlying http client #59

jgehrcke opened this issue Nov 28, 2019 · 7 comments · May be fixed by #132

Comments

@jgehrcke
Copy link

Is there a nice way here for exposing the additional configuration options for the underlying http client, such as timeout configuration?

@josnidhin
Copy link

josnidhin commented Dec 6, 2019

I was looking for alternatives to request package and came across this package based on the list shown at request/request#3143

Timeouts are something that I found missing when evaluating this package for my use case.

@mikeal
Copy link
Owner

mikeal commented Feb 12, 2020

timeout isn’t actually supported in node.js and fetch. there’s a socket timeout in node.js and AFAIK there isn’t a timeout in fetch either. however, there are numerous quality timeout libraries for promises that will work generically with bent so that’s your best option.

@mikeal mikeal closed this as completed Feb 12, 2020
@mpareja
Copy link

mpareja commented Feb 12, 2020

@mikeal Ancillary libraries for handling timeouts are insufficient without bent providing a mechanism to reclaim resources.

The request timeout implementation, for instance, aborts the request resulting in the TCP socket being torn down - that's important on a server. bent doesn't support setting a timeout nor gives us access to node's ClientRequest instance, so we won't have any way to tear down the TCP connection when using a timeout library.

Please consider re-opening this issue or warning others about this.

@mikeal
Copy link
Owner

mikeal commented Feb 12, 2020

I’m going to re-open. I hadn’t entirely recalled all the issues related to “cancellable promises” and aborting generally.

We may need to start returning new/special promises in order to do this properly

I’m still not sure that we’ll actually add a timeout feature, but we do need proper cancellation in order for users to be able to add a working timeout.

@mikeal mikeal reopened this Feb 12, 2020
@icetbr
Copy link

icetbr commented Feb 13, 2020

You probably know how to do it, but heres the implementation of a timeout anyway

nodejs/node#12005 (comment)

@mikeal
Copy link
Owner

mikeal commented Feb 13, 2020

heres the implementation of a timeout

we need to make sure we provide an API that works in browers as well. this gets tricky because of how fetch needs to be aborted.

@Templum
Copy link

Templum commented Jul 22, 2020

@mikeal Not so sure if an API makes sense here, given that as far as I can tell based on this fetch would require a specific property to be present. Similar to the nodejs specific solution. And with that required probably it is cleaner to have just a timeout property that in nodejs directly gets forwarded and for browser code get's wrapped.

Nevertheless as previously addressed it is an important feature, especially for the server side.

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 a pull request may close this issue.

6 participants