spdy-http2 / node-spdy

SPDY server on Node.js
2.81k stars 196 forks source link

TypeError: Cannot read property 'incoming' of null #252

Open ben-page opened 8 years ago

ben-page commented 8 years ago

I'm using node-spdy 3.2.3 on node 5.8.0. After pushing a couple files, spdy fails with this error.

TypeError: Cannot read property 'incoming' of null
    at ServerResponse.push (...\node_modules\spdy\lib\spdy\response.js:83:29)

lib\spdy\response.js

exports.push = function push(path, headers, callback) {
  var frame = {
    path: path,
    method: 'GET',
    status: 200,
    host: this.socket.parser.incoming.headers.host, //line 83
    headers: headers.request,
    response: headers.response
  };

  var stream = this.socket._handle._spdyState.stream;
  return stream.pushPromise(frame, callback);
};

I'm calling push() like this:

var stream = res.push(pushUrl, { response: headers });
stream.end(body);

If I step into res.push(), this is the ServerResponse object and socket is a Socket object, but parser is null.

ben-page commented 8 years ago

The problem is occasionally the ServerResponse is sent (and ended) before the push. After the ServerResponse has ended, the parser is gone (and probably other things, too).

This isn't a bug in the spdy module (although being able to push without depending on a ServerResponse object would be helpful). But, I think this caveat should probably be mentioned in the documentation.

anandsuresh commented 8 years ago

I seem to have a similar problem where in the underlying socket is closed and cleaned up, before the push stream is created. The way I see it, the culprit is this line: https://github.com/indutny/node-spdy/blob/master/lib/spdy/response.js#L83

Instead of depending on the parser to reference the IncomingMessage to grab the hostname, perhaps it would be better to use an alternate way of grabbing the hostname. Looking at https://github.com/indutny/node-spdy/blob/master/lib/spdy/server.js, it seems that a cleaner way to reference the request would be to get the request and response to reference each other (perhaps a weak reference, if needed; not sure of the dynamics there).

Something like the following in https://github.com/indutny/node-spdy/blob/master/lib/spdy/server.js#L229-L238

  req.isSpdy = true;
  req.spdyVersion = handle.getStream().connection.getVersion();
  req.spdyStream = handle.getStream();

  debug('override req/res');
  res.writeHead = spdy.response.writeHead;
  res.end = spdy.response.end;
  res.push = spdy.response.push;
  res.writeContinue = spdy.response.writeContinue;
  res.spdyStream = handle.getStream();

  req.res = res;   // Request gets a reference to its corresponding response stream
  res.req = req;   // Response gets a reference to its corresponding request stream

With this in place, .push() can then change to use this.req.headers.host to identify the host.

Does that seem alright @indutny?