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

terimnated connections should propagate into response.body #1056

Closed
wants to merge 1 commit into from

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?

@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
@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
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
Labels
bug-illustration Illustration of a bug via failing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants