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

[feat] Timeouts - response (TTFB) and deadline (total time) #1254

Closed
wants to merge 1 commit into from

Conversation

rook2pawn
Copy link

@rook2pawn rook2pawn commented Mar 2, 2018

Enable timeouts for http, both response (time to first byte) and deadline (total time) timeouts.

Description

Added a promise on the request in src/http.js that either resolves with the first byte or rejects if the response timeout has passed due to a setTimeout. Added a second promise on that waits on completion that does similar logic for deadline timeout.

Error errno/code matches superagent's error ECONNABORTED / ETIMEOUT / ETIME which lines up with standard protocol for such errors.

Motivation and Context

Adds timeout functionality features that were present in superagent

How Has This Been Tested?

test with TTFB and deadline timeouts included.

Screenshots (if appropriate):

Types of changes

  • No code changes (changes to documentation, CI, metadata, etc)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Non breaking change, only adds functionality.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Code style of project is adhered to. there were some trailing end of line spaces that were automatically deleted in the README from my whitespace plugin. Let me know what I can do to help make this PR a reality!

@char0n
Copy link
Member

char0n commented Jul 4, 2020

Thank you for bringing this up!

Fetch API still doesn't support native timeouts, but it looks like it will in future. This PR is fairly old and now fetch API support AbortController that can supplement timeout functionality.

Here is a code example from stackoverflow to demonstrate implementing timeouts using AbortController:

const controller = new AbortController();
const signal = controller.signal;

const fetchPromise = fetch(url, {signal});

// 5 second timeout:
const timeoutId = setTimeout(() => controller.abort(), 5000);


fetchPromise.then(response => {
  // completed request before timeout fired

  // If you only wanted to timeout the request, not the response, add:
  // clearTimeout(timeoutId);
})

With this and ability to override fetch function in our HTTP Client implementation I think this is purely a documentation issue. Having an example in documentation showing how to implement timeouts (if one needs them) would solve the problem for now.

@rook2pawn what is your opinion on that?

@char0n
Copy link
Member

char0n commented Sep 21, 2020

Closing in favor of #1727

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants