Skip to content

terimnated connections should propagate into response.body#1056

Closed
Gozala wants to merge 1 commit intonode-fetch:masterfrom
Gozala:premature-connection-close
Closed

terimnated connections should propagate into response.body#1056
Gozala wants to merge 1 commit intonode-fetch:masterfrom
Gozala:premature-connection-close

Conversation

@Gozala
Copy link
Contributor

@Gozala Gozala commented Jan 1, 2021

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • New feature
  • Other, please explain: Test case for the bug report

What changes did you make? (provide an overview)

  • Added a test case that illustrates the probem

Which issue (if any) does this pull request address?

#1055

Is there anything you'd like reviewers to know?

This was referenced Jan 1, 2021
Closed
Closed
@tekwiz
Copy link
Member

tekwiz commented Jan 2, 2021

@Gozala Is this intended to be a draft pull request? I only see a failing test case for Node 12. Also, it appears that Node 14 is unaffected by this bug. Can you confirm?

@tekwiz
Copy link
Member

tekwiz commented Jan 2, 2021

@Gozala Nevermind my previous comment. I understand what this is for now. Thank you for the test case. I may have a solution for this in the works already, but I'll keep you posted.

@tekwiz tekwiz added the bug-illustration Illustration of a bug via failing test label Jan 2, 2021
@tekwiz tekwiz mentioned this pull request Jan 3, 2021
Closed
@Gozala
Copy link
Contributor Author

Gozala commented Jan 4, 2021

@tekwiz I did some investigation of this and I think the fix would require lifting this, up into the response.body itself:

node-fetch/src/body.js

Lines 218 to 230 in d016690

if (body.readableEnded === true || body._readableState.ended === true) {
try {
if (accum.every(c => typeof c === 'string')) {
return Buffer.from(accum.join(''));
}
return Buffer.concat(accum, accumBytes);
} catch (error) {
throw new FetchError(`Could not create Buffer from response body for ${data.url}: ${error.message}`, 'system', error);
}
} else {
throw new FetchError(`Premature close of server response while trying to fetch ${data.url}`);
}

However I was not sure what made sense here as I don't exactly understand rationale behind choosing to use node stream for response.body (as opposed to ReadableStream) & if propagating those errors was aligned with it.

@tekwiz tekwiz mentioned this pull request Jan 10, 2021
Merged
4 tasks
@tekwiz
Copy link
Member

tekwiz commented Feb 28, 2021

This was merged with #1064.

@tekwiz tekwiz closed this Feb 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug-illustration Illustration of a bug via failing test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Gozala @tekwiz