Skip to content

Conversation

nightwing
Copy link
Contributor

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

engine.io often crashes with an error

TypeError: "list" argument must be an Array of Buffers
    at Function.Buffer.concat (buffer.js:314:13)
    at IncomingMessage.onData (node_modules/engine.io/lib/transports/polling.js:160:23)
    at emitOne (events.js:96:13)
    at IncomingMessage.emit (events.js:188:7)
    at IncomingMessage.Readable.read (_stream_readable.js:381:10)
    at flow (_stream_readable.js:761:34)
    at resume_ (_stream_readable.js:743:3)
    at _combinedTickCallback (internal/process/next_tick.js:80:11)
    at process._tickDomainCallback (internal/process/next_tick.js:128:9)

this is caused by a bug in new versions of node, where setEncoding call does not work for messages that are already in the queue
The following code snippet

console.log(process.version)

var stream = require('stream');
var a = new stream.PassThrough();

a.write(new Buffer("hello"));
a.write('foobar');

console.log('before handler');
a.setEncoding('utf8');
a.on('data', function(chunk) {
    console.log('data handler', chunk);
});
console.log('after handler');

a.write(new Buffer("hello"));
a.write('foobar');

in node 0.10 prints

v0.10.48
before handler
data handler hellofoobar
after handler
data handler hello
data handler foobar

and in newer versions of node prints

v6.11.2
before handler
after handler
data handler <Buffer 68 65 6c 6c 6f>
data handler <Buffer 66 6f 6f 62 61 72>
data handler hello
data handler foobar

this pull request makes sure that concat is never called in the non binary case, even if setEncoding does not work properly.

This is related to #305, which have fixed a part of the issue.

@darrachequesne darrachequesne merged commit 3dcc2d5 into socketio:master Aug 31, 2017
@darrachequesne
Copy link
Member

@nightwing nice, thanks for the clear explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants