Figure out how we are going to deal with potentially broken UTF8 #106

Closed
3rd-Eden opened this Issue Oct 24, 2012 · 8 comments

4 participants

@3rd-Eden

If we start sending over UTF8, the connection starts breaking down (if you are using websockets)

(Note, this uses the new streaming api that I'm working on)

'use strict';

// setup engine.io
var http = require('http').createServer()
  , engine = require('./index').attach(http)
  , fs = require('fs');

// engine.io client
var client = require('engine.io-client');

engine.on('connection', function (socket) {
  console.log('connection', __filename);
  var stream = fs.createReadStream('/dev/random');

  // Kill the stream, socket closed biatches
  socket.on('close', function () {
    stream.destroy();
  });

  stream.pipe(socket);
});

var socket = new client.Socket('ws://localhost:8080');

socket.onopen = function () {
  console.log('socket open', 'socket has been opened');
};

socket.onmessage = function (data) {
  console.log('received', data.data.length);
};

socket.onclose = function () {
  console.log('closed the socket');
};

// Output info
http.listen(8080);

The fix for this would be doing a JSON.stringify on the data before we send it over the WebSocket connection.

@rauchg

We should also check out what ServerResponse.write is doing with the buffer before it makes it to the net.Stream.

@rauchg

Actually we should look at what's making ws error out instead.

@mjgil

Hey guys, I just ran this with my own txt file and was wondering if this was the error to be fixed.

stream.js:66
dest.end();
^
TypeError: Object # has no method 'end'
at onend (stream.js:66:10)
at EventEmitter.emit (events.js:126:20)
at afterRead (fs.js:1330:12)
at Object.wrapper as oncomplete

@rauchg

@mjgil no, he's using an experimental stream branch he was working on. You should instead .send directly instead of using .pipe

@mjgil mjgil referenced this issue in socketio/engine.io-protocol Mar 17, 2013
Closed

adding first utf8 encoding fix #11

@rauchg

@3rd-Eden do you think that pull is a good fix ^ ?

@3rd-Eden

@guille it looks legit, but haven't tested it.

@defunctzombie

Was this fixed? Has the issue gone away?

@defunctzombie

Based on some recent commits I think this is resolved and should be closed. If there are issues discovered they should be new issues with failing cases.

@rauchg rauchg closed this May 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment