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

Set encoding of the request to utf8 before listening on data events #305

Merged

Conversation

lbdremy
Copy link
Contributor

@lbdremy lbdremy commented Feb 12, 2015

Hello,

I'm running into a case where the data event handler is called before the set encoding is called, in this case I end up having chunks as an empty string and data is a Buffer thus raising the error `Possibly unhandled TypeError: Object has no method 'copy'
I always thought the data event handler was called after the next tick, but it does not seem to be the case, or I am missing something.
Moving the setEncoding method before attaching the handler resolves my issue.

Thank

@nkzawa
Copy link
Contributor

nkzawa commented Mar 8, 2015

Hmm, it looks this PR doesn't change the behavior to me. Anyway, I'd like to reproduce the problem. How can I do that?

@STRML
Copy link

STRML commented Oct 8, 2015

We are seeing this in #356 and primus/primus#196 (comment). It is very hard to reproduce and I have not successfully been able to do so, but I am seeing it hit our production servers about once or twice a week. Last time I saw it, I was running iojs v2.5.0.

I am, however, able to reproduce the issue using the following script:

var stream = require('stream');

var a = new stream.PassThrough();

a.write('foobar');

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

a.setEncoding('utf8');

The correct output is:

before handler
after handler
data handler foobar

Every Node version > 0.10 has the correct behavior that I tested, but 0.10.40 does not:

before handler
data handler <Buffer 66 6f 6f 62 61 72>
after handler

Since engine.io still supports 0.10 it makes sense to merge this. I am not sure why I was seeing it in other versions - I will continue to investigate - but this is still a solid fix and needed for 0.10 users.

@STRML
Copy link

STRML commented Oct 8, 2015

For future reference, this appears to be the commit that fixed the bug, which landed in v0.11.5. It's also known as streams3. It makes the commit comment "There is no API change beyond this added flexibility" somewhat inaccurate: nodejs/node@0f8de5e#diff-ba6a0df0f5212f5cba5ca5179e209a17R709

@lpinca
Copy link
Contributor

lpinca commented Oct 8, 2015

@STRML good job!

@STRML
Copy link

STRML commented Oct 12, 2015

@nkzawa Could you please review?

@nkzawa
Copy link
Contributor

nkzawa commented Oct 12, 2015

@STRML 👍

rauchg added a commit that referenced this pull request Nov 16, 2015
…g-on-data

Set encoding of the request to utf8 before listening on data events
@rauchg rauchg merged commit fb811bf into socketio:master Nov 16, 2015
@rauchg
Copy link
Contributor

rauchg commented Nov 16, 2015

Awesome! Thank you :)

@STRML
Copy link

STRML commented Nov 16, 2015

Thanks @rauchg, had this biting us at random for a long time now.

@rauchg
Copy link
Contributor

rauchg commented Nov 16, 2015

Yep, I'll get this published asap

darrachequesne pushed a commit that referenced this pull request May 8, 2020
Switch from depending on a tarball URL to the published
component-emitter package at its latest version.

Change all references to emitter module to the new
component-emitter name.
darrachequesne pushed a commit that referenced this pull request May 8, 2020
Upgrade component-emitter to 1.1.2, fix #305
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.

None yet

5 participants