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
Closed

Websockets now broken. #1405

gfwilliams opened this issue May 9, 2018 · 8 comments

Comments

@gfwilliams
Copy link
Member

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
Copy link
Contributor

opichals 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
Copy link
Member Author

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
Copy link

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
Copy link
Member

@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
Copy link

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
Copy link
Member

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
Copy link
Member

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
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
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
Copy link
Contributor

@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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants