JustinTulloss / zeromq.node

Node.js bindings to the zeromq library
MIT License
1.65k stars 284 forks source link

When no connections are made, _outgoing grows infinitely #271

Open ronkorving opened 10 years ago

ronkorving commented 10 years ago

The Socket._outgoing array has no limit to where it can grow. The scenario where it grows indefinitely (effectively leaking memory) is when the socket is never asked to connect anywhere. While this seems incredibly unlikely, I encountered such a case.

To avoid indefinite growth, a sane limit would be to follow the ZMQ_SNDHWM value.

Code snippet to do this:

/**
 * Send the given `msg`.
 *
 * @param {String|Buffer|Array} msg
 * @param {Number} flags
 * @return {Socket} for chaining
 * @api public
 */

Socket.prototype.send = function(msg, flags) {
  var hwm = this.getsockopt(zmq.ZMQ_SNDHWM);

  if (Array.isArray(msg)) {
    if (this._outgoing.length + msg.length >= hwm) {
      this.emit('error', new Error('Dropped message, high water mark of ' + hwm + ' reached'));
      return this;
    }

    for (var i = 0, len = msg.length; i < len; i++) {
      this.send(msg[i], i < len - 1
        ? zmq.ZMQ_SNDMORE
        : flags);
    }
  } else {
    if (this._outgoing.length + 1 >= hwm) {
      this.emit('error', new Error('Dropped message, high water mark of ' + hwm + ' reached'));
      return this;
    }

    if (!Buffer.isBuffer(msg)) msg = new Buffer(String(msg), 'utf8');
    this._outgoing.push([msg, flags || 0]);
    if (!(flags & zmq.ZMQ_SNDMORE)) this._flush();
  }

  return this;
};

It just seems a bit inefficient to re-read that socket option on every single send. If anybody has any suggestions to how we could be a bit more efficient about this, I would love to hear them.

kkoopa commented 10 years ago

What to do with REQ, DEALER, PUSH, PULL and PAIR sockets? According to the specs they should block when entering the mute state due to SNDHWM, this is obviously not desired.

STREAM sockets should do EAGAIN in the same condition, according to the specs.

kkoopa commented 10 years ago

174 is related

ronkorving commented 10 years ago

Maybe we should breathe some new life into #174. Perhaps I should just get that merged, so people can give it a shot.

soplwang commented 10 years ago

174 isn't intent to resolve this issue. This issue is all about flow-control. My suggest is follow node.js' flow control pattern (pause/resume or read() for Readable, write()/drain for Writable stream).