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

Handle connectivity issues while reading the body #1343

Merged
merged 12 commits into from
Nov 10, 2020
Merged

Handle connectivity issues while reading the body #1343

merged 12 commits into from
Nov 10, 2020

Conversation

delvedor
Copy link
Member

@delvedor delvedor commented Nov 3, 2020

Until now the client was not verifying the content-length header, this pr address this issue.
For supporting this, the unzip process has been moved from the Connection class to the Transport class.

A byproduct of this pr is the fix of #1088.

Closes: #1088
/cc: @msisti @tomsquest

@delvedor
Copy link
Member Author

delvedor commented Nov 3, 2020

It looks like there is a problem with Node v14, looking into it.

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

Left a small comment, but otherwise looks good to me

cleanListeners()
this._openRequests--
request.once('error', () => {}) // we need to catch the request aborted error
request.abort()
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use request.destroy() it might solve the v14 problem and it will also not emit an error event so you will no longer need the empty error handler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review! I'm looking into this, as request.abort has been deprecated in newer versions of node.
Very likely I'll address this in a separate pr while working on #1297.

@delvedor
Copy link
Member Author

delvedor commented Nov 6, 2020

Thanks to the awesome Node.js contributors I've discovered the response.on('aborted') event, which among other things, addresses specifically this issue. I've removed the content length check as the aborted event always supersede it.
The best part is that it works in the same way from Node.js 10 and above.

@delvedor delvedor changed the title Verify content length Handle connectivity issues while reading the body Nov 10, 2020
@delvedor delvedor merged commit edd4f78 into master Nov 10, 2020
@delvedor delvedor deleted the fix-1088 branch November 10, 2020 17:16
@github-actions
Copy link
Contributor

The backport to 7.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-7.x 7.x
# Navigate to the new working tree
cd .worktrees/backport-7.x
# Create a new branch
git switch --create backport-1343-to-7.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick ---mainline 1 edd4f78badb47b96120d60e36f0c0f16dd7584e6
# Push it to GitHub
git push --set-upstream origin backport-1343-to-7.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-7.x

Then, create a pull request where the base branch is 7.x and the compare/head branch is backport-1343-to-7.x.

lib/Transport.js Outdated
@@ -237,9 +237,9 @@ class Transport {
if (isCompressed) {
payload = Buffer.concat(payload)
}
if (result.headers['content-length'] !== undefined && Number(result.headers['content-length']) !== payload.length) {
if (result.headers['content-length'] !== undefined && Number(result.headers['content-length']) !== Buffer.byteLength(payload)) {

Choose a reason for hiding this comment

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

That´s why everyone loves javascript 👯‍♀️

@Spacefish
Copy link

We have the same issue.. I noticed something strange... For all requests failing the _source: reply from ES to Kibana contains a 3 bytes UTF-8 Bom inside the JSON string.. not sure if this is even legal:
image

@SSANSH
Copy link

SSANSH commented Dec 28, 2021

hi, possible to back port this fix on 5.x ?

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.

DeserializationError: Unexpected end of JSON input
4 participants