add functions to change timeout

[UziTech]

add functions to req to change timeout

req.resetTimeout(delay): reset the timeout and start from now
req.addTimeout(delay): add to the current timeout
req.getTimeout(): get the amount of milliseconds left (0 after timedout)

fixes #26

[dougwilson]

Hi @UziTech thanks for the pull request! I'll start reviewing over the coming days. The following jumped out at me just with an initial glance at the pull request, though:

1. There is no documentation. Some folks wait to write documentation after the review, but you didn't mention that in your description, so wasn't sure if you were waiting or accidentally forgot to write it.
2. req.setTimeout already exists -- https://nodejs.org/dist/latest-v10.x/docs/api/http.html#http_message_settimeout_msecs_callback . You'll need to use a different method name as not to overwrite core Node.js method and break existing apps (and just otherwise prevent apps from being able to access this functionality).
3. It doesn't seem like req.clearTimeout() will stop req.timeoutLeft() from continuing to count down as if the timeout was still there.

[UziTech]

Good points.

1. I figured a few things would change so I was waiting on the docs.
2. What would you like it to be called? A couple suggestions:
   a. .startTimeout()
   b. .restartTimeout()
   c. .timeout()
3. Nice catch, I will fix that.

[UziTech]

also a thought: should .timeoutLeft() be changed to .timeoutRemaining()

P.S. It's a good thing this problem doesn't involve cache invalidation.

[UziTech]

I renamed the functions:

req.setTimeout(...) -> req.resetTimeout(...)
req.timeoutLeft(...) -> req.getTimeout(...)

and added docs to the README

[UziTech]

@dougwilson this should be good to go.

[UziTech]

It would be great to get this merged.

/cc @dougwilson @juliangruber @jonathanong

[dougwilson]

Hi @UziTech sorry. There are still a few issues with the PR and I was just going to resolve them for you instead of doing more back and forth, I just haven't gotten to it yet.

[UziTech]

@dougwilson is this still moving forward? I have been using this in production with @uzitech/connect-timeout for a while and I haven't had any issues.

[UziTech]

@dougwilson are you still able to merge this?

[UziTech]

Over a year and a half waiting for this PR to be merged!

@expressjs If you need help maintaining some of the expressjs packages I would be willing to help.

[UziTech]

We are almost at the 2 year mark. Is there anything I can do to move this along?

[dougwilson]

Hi @UziTech , I think that with a new sweeping overview we are doing, this module is likely to just be discontinued and deprecated, as it will not work at all with the upcoming Express, as it calls next() more than once and other design issues. I think we should just be real at this point: if this is something you need / want, just publish the module as a fork and go from there; even the name of this module is problematic for our organization, having the connect- prefix.

[UziTech]

Thanks for the response.

[dougwilson]

And @UziTech I hope you can accept my apology for letting this sit here -- I really didn't want to continue this module in many ways and the original authors didn't seem to be around responsive, either. If it does get a fresh breath of air, having the module not start with connect- would be awesome anyhow, as it makes little sense with Express since it hasn't been built on connect in years... I do have a couple legacy apps that use this module still, but there are design issues that makes it hard to use; the readme shows putting a "stopper" middleware between everything to at least make it kind of work, but it... sucks :(

It may make sense to rebirth as like a middleware wrapper instead. To do something like app.use(timeout(200, myMiddleware)) that would time out that middleware at 200ms, IDK. I haven't put too much thought into it, and I think that is part of the problem--it deserves to be in the hands of someone who can really put thought into it...

[UziTech]

No problem. I just want to make clear that, whatever timeout method is used, it is important to have the ability to cancel/extend the timeout in certain situations.

In my project a middleware wrapper wouldn't work because there are multiple middleware that could cause a timeout, so it works better to set a timeout over the entire request.

As I said above, If you need any help maintaining small projects I would be happy to help 😁 👍