spdy-http2 / node-spdy

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

Unexpected connection close handling in Agent #165

Closed fuson closed 8 years ago

fuson commented 10 years ago

As i see there is no way to handle connection failed on tcp level, when f.e. server crashed. Ping has no timeout too. What do you think about reconnect logic?

indutny commented 10 years ago

I think the agent should just listen for close event an re-emit it. Mind submitting PR for this?

fuson commented 10 years ago

Listen on what emitter? socket / connection?

In Agent i can simply listen 'error' event, but i dont see event about successfull connection create? f.e.

agent = spdy.createAgent host: 'localhost' port: 1337 spdy: plain: true ssl: true

agent.on 'error', (error) -> console.log error.code agent.on 'XXXX', (socket) -> HERE is the place where i know about connection success

... in this place i dont know about success until i receive 'error'

fuson commented 10 years ago

I found one bad issue is node.js (0.10.29). Look at https createConnection

function createConnection(port, host, options) {
  if (typeof port === 'object') {
    options = port;
  } else if (typeof host === 'object') {
    options = host;
  } else if (typeof options === 'object') {
    options = options;
  } else {
    options = {};
  }

  if (typeof port === 'number') {
    options.port = port;
  }

  if (typeof host === 'string') {
    options.host = host;
  }

  return tls.connect(options);
}

There is no way to attach callback into tls.connect!!! In this case there is no good way to create reconnection logic with proper announce on connection create and bind on it some recovery logic (for example resending lost http requests).

For my purpose i can hack it, embedd rewrited createConnection function with proper call, something like:

this.createConnection = function (port, host, options, callback) {
      if (typeof port === 'object') {
          callback = host;
          options = port;
      } else if (typeof host === 'object') {
          callback = options;
          options = host;
      } else if (typeof options === 'object') {
          options = options;
      } else {
          options = {};
      }

      if (typeof port === 'number') {
          options.port = port;
      }

      if (typeof host === 'string') {
          options.host = host;
      }

      return tls.connect(options, callback);
  };

and

// TODO(indutny): Think about falling back to http/https
  var socket = createConnection.call(this, util._extend({
    NPNProtocols: ['spdy/3.1', 'spdy/3', 'spdy/2'],
    ALPNProtocols: ['spdy/3.1', 'spdy/3', 'spdy/2']
  }, this.options), function () {self.emit('connect', socket);});  

But, is it proper way in general?

indutny commented 10 years ago

Sorry, I don't really follow your thoughts here :) You could listen for connect event on a socket. Technically this is how connect callback works internally.