express-ws crashes if I use it with express-session.

[sunaemon]

When I run the following code and access to http://localhost:3000/ with Firefox, It crashed.

const express = require('express');
const cookieParser = require('cookie-parser');
const session = require('express-session');
const app = express();
const expressWs = require('express-ws')(app);

app.use(cookieParser());
app.use(session({ secret: 'foo' }));

app.use('/', function(req, res) {
    res.send("<script>new WebSocket('ws://localhost:3000/bar');</script>");
});

app.listen(3000);

express-session deprecated undefined resave option; provide resave option app.js:8:9
express-session deprecated undefined saveUninitialized option; provide saveUninitialized option app.js:8:9
_http_outgoing.js:144
      this.outputSize += this._header.length;
                                     ^

TypeError: Cannot read property 'length' of null
    at ServerResponse._send (_http_outgoing.js:144:38)
    at ServerResponse.write (_http_outgoing.js:536:16)
    at ServerResponse.end (_http_outgoing.js:635:10)
    at writeend (/Users/sunaemon/Dropbox/myapp/node_modules/express-session/index.js:261:22)
    at Immediate.onsave (/Users/sunaemon/Dropbox/myapp/node_modules/express-session/index.js:335:11)
    at runCallback (timers.js:666:20)
    at tryOnImmediate (timers.js:639:5)
    at processImmediate [as _immediateCallback] (timers.js:611:5)

I'm using node v6.10.0 and following packages. cookie-parser@1.4.3 express@4.14.1 express-session@1.15.1 express-ws@3.0.0

Is this a bug of express-ws? Or should I send this issue for express-session? The error message is similar to #46, but I can not determine whether this issue is related to it or not.

[joepie91]

The culprit is likely this bit of code:

app.use('/', function(req, res) {
    res.send("<script>new WebSocket('ws://localhost:3000/bar');</script>");
});

Because you're using app.use rather than app.get, it executes that route for every request starting with /, which in effect means it's executed for every request in the entire application, including the 'fake' route that express-ws generates.

Can you try to change app.use to app.get and see if the problem persists? If that change makes the problem go away, then it's probably the same issue that I've been suspecting in #46, and I'd have to look at whether this is fixable. While having a wildcard/site-wide route is usually a mistake, there can also be legitimate reasons for it, so it should really be supported.

[joepie91]

Aside: if it turns out that switching app.use to app.get fixes it, this should probably be closed and treated as a duplicate of #64.

[sunaemon]

I replace app.use with app.get, it works! Thanks. But it seems that I failed to reproduce same error in my code. Sorry.

The following code emits the same error as my first post.

'use strict';

const express = require('express');
const path = require('path');
const cookieParser = require('cookie-parser');
const session = require('express-session');

const app = express();
const expressWs = require('express-ws')(app);

app.set('views', path.join(__dirname, 'views'));
app.set('view engine', 'pug');

app.use(cookieParser());
app.use(session({
    secret: 'bar',
    resave: false,
    saveUninitialized: true
}));

app.get('/', function(req, res) {
    res.send('<script>new WebSocket("ws://127.0.0.1:3000/foo");</script>');
});

// catch 404 and forward to error handler
app.use(function(req, res, next) {
    const err = new Error('Not Found');
    err.status = 404;
    next(err);
});

// error handler
app.use(function(err, req, res, _next) {
//    if (req.ws)    // WE NEED TO IGNORE EXPRESS-WS DUMMY REQUEST
//        return;
    // set locals, only providing error in development
    res.locals.message = err.message;
    res.locals.error = req.app.get('env') === 'development' ? err : {};

    // render the error page
    res.status(err.status || 500);
    res.render('error');
});

module.exports = app;

It seems that the 404 catcher and the error handler, which express4 generates, are the cause of error. I could get rid of the error by commenting out the following lines:

//    if (req.ws)    // WE NEED TO IGNORE EXPRESS-WS DUMMY REQUEST
//        return;

Since the functions express4 generates leads to invalid implementation, I would be glad if this workaround be written in README.md of this project.

[Kiikurage]

How about adding "Not-Found" handler for WebSocket?

const express = require('express');
const cookieParser = require('cookie-parser');
const session = require('express-session');
const app = express();
const expressWs = require('express-ws')(app);

app.use(cookieParser());
app.use(session({
    secret: 'bar',
    resave: false,
    saveUninitialized: true
}));

app.ws('*', (ws, req) => ws.send('Not Found'));

app.get('/', (req, res) => {
    res.send('<script>new WebSocket("ws://127.0.0.1:3000/foo");</script>');
});

app.use((req, res, next) => {
    const err = new Error('Not Found');
    err.status = 404;
    next(err);
});

app.use((err, req, res, _next) => res.send('hoge'));

app.listen(3000);

[joepie91]

@sunaemon Well, ideally, no such workaround should be necessary in the first place - express-ws just shouldn't interfere with normal operation like that, and this is a bug in express-ws that needs to be fixed. This is definitely a duplicate of #64 though, so let's move over there. (@HenningM: Can you close this one?)

@Kiikurage I'm not sure what problem that's meant to solve?

[Kiikurage]

@joepie91 As you said, I think plugins shouldn't interfere with default operation too, and so, with using my workaround, developers only need to edit their websocket route implementation and need not to edit default app.js code generated by express-generator. of course, as you said, it is better solution to fix express-ws not to be handled by HTTP middlewares, i think.

Btw, if it would be fixed, then how we should handle "Not Found" errors?

[sunaemon]

@joepie91 I see. I agree that this issue is duplication of #64, and we should move there. I'm closing this issue.