fix: set request timeout to 4s by default

[pmilliotte]

Description of changes:

Viewer request lambda edge timeout max is 5s which leads to unhandled exceptions when https requests take longer. Setting a default timeout for https request to 4s is a first step in avoiding requests timeout leading to unhandled errors such as:

Next step would be to anticipate lambda timeout with getRemainingTimeInMillis() context method and set the request timeout to this number minus a few milliseconds.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[ottokruse]

Great idea to add the timeout!

For another lib I worked on, I once looked into this and added a default timeout too. What I found then was that the timeout that you can pass to the RequestOptions (which your PR is adding) is actually the socket idle timeout, i.e. not the total round trip time-out (which is what you want to cap).

Your PR is still an improvement over having no timeout at all, but could be improved by instead (or also) adding a total round trip time-out as we did here: https://github.com/awslabs/aws-jwt-verify/blob/main/src/https-node.ts#L57

We used a "dumb" setTimeout there but I think the more modern way currently is to use AbortSignal.timeout(3000) and pass that to the request options. Can you give that a shot in this PR? If it works you might do a similar PR against https://github.com/awslabs/aws-jwt-verify/ if you want :)

[ottokruse]

This works but I think we can simplify it.

If you look at the code as it was before, on L18:

const req = request(uri, options ?? {}, (res) =>

We can change that to:

const req = request(uri, { signal: AbortSignal.timeout(DEFAULT_REQUEST_TIMEOUT), ...options }, (res) =>

And done!

No need to have two promises race against each other

[pmilliotte]

This works but I think we can simplify it.

If you look at the code as it was before, on L18:

const req = request(uri, options ?? {}, (res) =>

We can change that to:

const req = request(uri, { signal: AbortSignal.timeout(DEFAULT_REQUEST_TIMEOUT), ...options }, (res) =>

And done!

No need to have two promises race against each other

Nice, thanks for the tips ! I did not know about AbortSignal.timeout().

[ottokruse]

LGTM 🎉

Thanks!