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

fix: do not setup the response timeout if the stream has already ended #53

Merged
merged 1 commit into from
Mar 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lib/body.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,10 @@ class Body {
: this.size ? new MinipassSized({ size: this.size })
: new Minipass()

// allow timeout on slow response body
const resTimeout = this.timeout ? setTimeout(() => {
// allow timeout on slow response body, but only if the stream is still writable. this
// makes the timeout center on the socket stream from lib/index.js rather than the
// intermediary minipass stream we create to receive the data
const resTimeout = this.timeout && stream.writable ? setTimeout(() => {
stream.emit('error', new FetchError(
`Response timeout while trying to fetch ${
this.url} (over ${this.timeout}ms)`, 'body-timeout'))
Expand Down
27 changes: 27 additions & 0 deletions test/body.js
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,33 @@ t.test('stream body too slow', async t => {
})
})

t.test('no timeout if stream ends before we even start consuming', async t => {
// this test mimics how lib/index.js writes data into the intermediary minipass stream

// the SlowMinipass class delays the result from concat() mimicking a slow pipe downstream
class SlowMinipass extends Minipass {
async concat () {
// 10 millisecond delay before resolving
await new Promise((resolve) => setTimeout(resolve, 10))
return super.concat()
}
}
const networkStream = new Minipass()
const wrappedStream = new SlowMinipass()

networkStream.on('data', (chunk) => wrappedStream.write(chunk))
networkStream.on('end', () => wrappedStream.end())

for (let i = 0; i < 10; ++i) {
networkStream.write('some data')
}
networkStream.end()

// timeout of 1ms, must be lower than the 10ms used in SlowMinipass to trigger the bug
const b = new Body(wrappedStream, { timeout: 1 })
await t.resolves(b.text(), 'some data')
})

t.test('random toString-ing thing body', async t => {
const b = new Body({ toString () {
return 'a=1'
Expand Down