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

Websockets now broken. #1405

Closed
gfwilliams opened this issue May 9, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@gfwilliams
Copy link
Member

commented May 9, 2018

Hi @opichals,

It looks like the recent HTTP chunked PR has broken websocket support. There's a post on it here: http://forum.espruino.com/conversations/320553/

Basically run this on node.js:

var WebSocketServer = require('ws').Server,
    wss = new WebSocketServer({
        port: 8080
    });

console.log("Server on 8080");

wss.on('connection', function connection(ws) {
    ws.room = [];
    ws.send("User Joined");

    ws.on('message', function(message) {
        message = JSON.parse(message);
        if (message.join) {
            ws.room.push(message.join);
        }
        if (message.room) {
            broadcast(message);
        }
        if (message.msg) {
            console.log("Server got: " + message.msg);
        }
    });

    ws.on('error', function(er) {
        console.log(er);
    })


    ws.on('close', function() {
        console.log('Connection closed')
    })
});

function broadcast(message) {
    wss.clients.forEach(function each(client) {
        if (client.room.indexOf(message.room) > -1 || message.room == 'all') {
            client.send(message.msg);
        }
    });
}

and this on Espruino (you can do it on the Linux build if you copy ws.js/ws.min.js to a directory called node_modules):

var host = "localhost";
var WebSocket = require("ws");
    var ws = new WebSocket(host,{
      path: '/',
      port: 8080, // default is 80
      protocol : "echo-protocol", // websocket protocol name (default is none)
      protocolVersion: 13, // websocket protocol version, default is 13
      origin: 'Espruino',
      keepAlive: 60,
      headers:{ some:'header', 'ultimate-question':42 } // websocket headers to be used e.g. for auth (default is none)
    });

ws.on('open', function() {
  console.log("Connected to server");
});

ws.on('message', function(msg) {
  console.log("MSG: " + msg);
});

Then you get RangeError: Invalid WebSocket frame: RSV1 must be clear from node.js

ws.js doesn't actually get around to sending anything, and it looks like require("net").connect calls back twice now, which causes onConnect/handshake in ws.js to be called twice, which breaks everything.

Could it be that it's now trying to parse headers even on connections that aren't HTTP?

It'd be good to get a fix out for this soon as it looks like it's broken in the 1v97 release :(

opichals added a commit to opichals/Espruino that referenced this issue May 9, 2018

opichals added a commit to opichals/Espruino that referenced this issue May 9, 2018

@opichals

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

@gfwilliams I am really sorry to have caused a bad release. I hope that the benefits outweigh the trouble caused.

Thank you for the detailed repro instructions. A fix is ready in #1408.

@gfwilliams

This comment has been minimized.

Copy link
Member Author

commented May 10, 2018

No problem - I should have had tests that covered it better, so thanks for tweaking the existing net test as well.

And yes, the benefits of your commits absolutely outweigh this one very small issue :)

@Jason-CloudTP

This comment has been minimized.

Copy link

commented May 10, 2018

this is still an issue in the latest release. i cannot fully test, as i cannot complete my onInit() code. in wifi 1v97.62, i cannot complete a connection with espruino as the ws server and a webpage as the ws client. this does work in 1v96.

@wilberforce

This comment has been minimized.

Copy link
Member

commented May 10, 2018

@Jason-CloudTP

Are you able to provide a simple test case with source files (use gist?) so that the issue can be produced easily to aid debugging?

@Jason-CloudTP

This comment has been minimized.

Copy link

commented May 11, 2018

sure. when using the server url in a browser, the client never connects via websocket.

`var WebSocket = require("ws");

var WIFI_NAME = "";
var WIFI_OPTIONS = { password : "" };

var wifi;

function onInit() {
wifi = require("Wifi");
wifi.connect(WIFI_NAME, WIFI_OPTIONS, function(err) {
if (err) {
console.log("Connection error: "+err);
return;
}
console.log("Connected!");
wifi.getIP(function (err, ip) {
console.log(JSON.stringify(ip));
});
startServer();
});
}

var page = '<script>var ws;setTimeout(function(){';
page += 'ws = new WebSocket("ws://" + location.host + "/my_websocket", "protocolOne");';
page += 'ws.onmessage = function (event) { console.log("MSG:"+event.data); };';
page += 'setTimeout(function() { ws.send("Hello to Espruino!"); }, 1000);';
page += '},1000);</script>';

function onPageRequest(req, res) {
res.writeHead(200, {'Content-Type': 'text/html'});
res.end(page);
}

function startServer() {
server = require('ws').createServer(onPageRequest);
// server = require('ws').createServer();
server.listen(8000);
server.on("websocket", function(ws) {
ws.on('message',function(msg) { print("[WS] "+ msg); });
ws.send("Hello from Espruino!");
});
}`

@wilberforce

This comment has been minimized.

Copy link
Member

commented May 11, 2018

I can confirm that the code fails on the unix build:

var page = '<script>var ws;setTimeout(function(){';
page += 'ws = new WebSocket("ws://" + location.host + "/my_websocket", "protocolOne");';
page += 'ws.onmessage = function (event) { console.log("MSG:"+event.data); };';
page += 'setTimeout(function() { ws.send("Hello to Espruino!"); }, 1000);';
page += '},1000);</script>';

function onPageRequest(req, res) {
res.writeHead(200, {'Content-Type': 'text/html'});
res.end(page);
}

function startServer() {
server = require('ws').createServer(onPageRequest);
// server = require('ws').createServer();
server.listen(8000);
server.on("websocket", function(ws) {
ws.on('message',function(msg) { print("[WS] "+ msg); });
ws.send("Hello from Espruino!");
});
}
startServer();

image

image

@wilberforce wilberforce reopened this May 11, 2018

@wilberforce

This comment has been minimized.

Copy link
Member

commented May 11, 2018

The code above works on:

git checkout RELEASE_1V96

in the console in chrome:

MSG:Hello from Espruino!

opichals added a commit to opichals/Espruino that referenced this issue May 12, 2018

Fix large HTTP headers receive.
Added a test which sends HTTP headers which take more than
MSS bytes (536) and therefore the httpParseHeaders() needs to be
called later again after the next packet arrives.

Fixes espruino#1405

opichals added a commit to opichals/Espruino that referenced this issue May 12, 2018

Fix receiving of large HTTP headers
Added a test which sends HTTP headers which take more than
MSS bytes (536) and therefore the httpParseHeaders() needs to be
called later again after the next packet arrives.

Fixes espruino#1405
@opichals

This comment has been minimized.

Copy link
Contributor

commented May 12, 2018

@wilberforce Thanks for isolating it. Fix is in #1415.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.