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

Unpipe lengthAccumulator to prevent 'write after end' error #1549

Merged
merged 2 commits into from
Jul 6, 2017

Conversation

mrtcode
Copy link
Contributor

@mrtcode mrtcode commented Jun 5, 2017

lengthAccumulator must be unpiped from httpStream otherwise 'write after end' error is thrown, which is impossible to handle. It seems that httpStream tries to write to lengthAccumulator even after lengthAccumulator.end() is called.

Here is all you need to reproduce the error:

let AWS = require('aws-sdk');

let s3 = new AWS.S3({
	httpOptions: {
		timeout: 1000
	}
});

let stream = s3.getObject({Bucket: 'bucket', Key: 'key'}).createReadStream();
stream.on('error', function() {
});

setTimeout(function() {}, 99999999);

Results to:

events.js:163
      throw er; // Unhandled 'error' event
      ^

Error: write after end
    at writeAfterEnd (_stream_writable.js:191:12)
    at PassThrough.Writable.write (_stream_writable.js:238:5)
    at IncomingMessage.ondata (_stream_readable.js:557:20)
    at emitOne (events.js:96:13)
    at IncomingMessage.emit (events.js:191:7)
    at IncomingMessage.Readable.read (_stream_readable.js:383:10)
    at flow (_stream_readable.js:763:34)
    at resume_ (_stream_readable.js:745:3)
    at _combinedTickCallback (internal/process/next_tick.js:80:11)
    at process._tickDomainCallback (internal/process/next_tick.js:128:9)

Node.js version v7.10.0

@mrtcode mrtcode changed the title lengthAccumulator responsible for 'write after end' error Unpipe lengthAccumulator to prevent 'write after end' error Jun 5, 2017
@codecov-io
Copy link

codecov-io commented Jun 5, 2017

Codecov Report

Merging #1549 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1549      +/-   ##
==========================================
+ Coverage   95.41%   95.47%   +0.06%     
==========================================
  Files         182      182              
  Lines        6409     6438      +29     
  Branches     1311     1321      +10     
==========================================
+ Hits         6115     6147      +32     
+ Misses        294      291       -3
Impacted Files Coverage Δ
lib/request.js 96.98% <100%> (+0.01%) ⬆️
clients/iot.js 100% <0%> (ø) ⬆️
lib/s3/managed_upload.js 93.84% <0%> (+1.35%) ⬆️
lib/credentials/ecs_credentials.js 94.02% <0%> (+2.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fcf374...9eaa1b0. Read the comment docs.

@jeskew
Copy link
Contributor

jeskew commented Jun 6, 2017

Hi @mrtcode,

Thanks for your contribution! The change looks good to me, but could you add a unit test to catch any future regressions?

@mrtcode
Copy link
Contributor Author

mrtcode commented Jun 7, 2017

Hello @jeskew,

It seems that this error occurs only when enough data is sent from server. It probably happens when internal buffers are full and time out error occurs. I think you need to find the actual reason why data is written to stream even after error. There can be more problems related to this.

@lock
Copy link

lock bot commented Sep 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants