Skip to content

add abort controller#114

Open
btrdnch wants to merge 7 commits intobitfinexcom:masterfrom
btrdnch:connection-timeout
Open

add abort controller#114
btrdnch wants to merge 7 commits intobitfinexcom:masterfrom
btrdnch:connection-timeout

Conversation

@btrdnch
Copy link
Copy Markdown

@btrdnch btrdnch commented Apr 13, 2026

Description:

use abort controller to avoid infinite response await for cases when connection could not be established
replace timeout logic with AbortController, see docs

Breaking changes:

  • minimal node version set to 16

New features:

  • [ ]

Fixes:

  • infinite response awaiting issue when server is down

PR status:

  • Version bumped
  • Change-log updated
  • Tests added or updated
  • Documentation updated

Copy link
Copy Markdown
Contributor

@Thomas-Heniart Thomas-Heniart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor suggestion

Comment thread lib/rest2.js Outdated
acamaragl
acamaragl previously approved these changes Apr 14, 2026
Co-authored-by: Thomas Heniart <heniart.thomas@gmail.com>
acamaragl
acamaragl previously approved these changes Apr 17, 2026
Comment thread lib/rest2.js Outdated

async _request (url, reqOpts, transformer, cb) {
let timeoutId
reqOpts.timeout = 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we forcing this to 0? and working with this._timeout?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because of this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think how we can do is this:

const timeout = reqOpts.timeout || this._timeout || BASE_TIMEOUT
delete reqOpts.timeout

this way you don't break anything and you can do patch release instead of mayor

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this._timeout already takes into account BASE_TIMEOUT

user-set timeout is passed only on instance creation and stored in this._timeout, which is later used as reqOpts.timeout, for instance here. this._timeout is already used as reqOpts.timeout everywhere, not sure why it's been implemented like this honestly.

lastly, it is major release because we bump node version. Earlier versions don't have abort controller.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my point is you're taking ability of user to set it's own timeout, we can still support that with this:

const timeout = reqOpts.timeout || this._timeout
delete reqOpts.timeout

if (this._timeout > 0) {
...
}

so it allows to set custom timeout per request but also supports default one

Comment thread lib/rest2.js
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 this pull request may close these issues.

5 participants