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

Not working on http.ServerResponse closing since Node 0.6.7 #9

Open
rstuven opened this issue Jun 12, 2012 · 2 comments
Open

Not working on http.ServerResponse closing since Node 0.6.7 #9

rstuven opened this issue Jun 12, 2012 · 2 comments

Comments

@rstuven
Copy link

rstuven commented Jun 12, 2012

Due to a change in Node http module, there is a loss of scope in a wrapped callback, resulting in an error.

@rstuven
Copy link
Author

rstuven commented Jun 12, 2012

A workaround is to monkey patch Node:

         var ServerResponse = require('http').ServerResponse;
         ServerResponse.prototype.assignSocket = function(socket) {
           var that = socket._httpMessage = this;
           this.onServerResponseClose = function onServerResponseClose() {
               that.emit('close');
           };
           socket.on('close', this.onServerResponseClose);
           this.socket = socket;
           this.connection = socket;
           this._flush();
         };
         ServerResponse.prototype.detachSocket = function(socket) {
           socket.removeListener('close', this.onServerResponseClose);
           socket._httpMessage = null;
           this.socket = this.connection = null;
         };

@CrabBot
Copy link

CrabBot commented Nov 9, 2012

I manage the trycatch module (async try/catch), and I use Tom's long-stack-traces technique. I came across this issue as well, but it wasn't an option to turn off in production b/c we use trycatch for stability.

It turns out the issue is related to how long-stack-traces' (LST) shims functions, EventEmitter specifically. If you're not familiar with LST's internals, what it basically does is the following:

  1. Shim node.js' (JavaScript's) async functions
  2. In the shims, shim/wrap callbacks before passing them to original function so we can wrap a try/catch around the invocation of the callback

Unfortunately this breaks EventEmitter.removeListener b/c removeListener uses a === to pop event handlers from the stack, and the functions you pass removeListener are not the same that were passed to the original addListener / on and are thus not removed. This creates both a memory leak, and situations like this where node.js internals expect a specific remove to resolve before calling emit.

That's why this patch works, b/c you're effectively preventing the _httpMessage from being GC'd, but the problem is you're emitting a close event that should never fire. At any rate, for you guys to resolve this in LST, you'd need to implement something similar to what I'm doing in trycatch:

https://github.com/CrabDude/trycatch/blob/master/lib/hook.js#L40-60

// Wrap EventEmitters
var EventEmitter = require('events').EventEmitter
var onEvent = EventEmitter.prototype.on
EventEmitter.prototype.on = EventEmitter.prototype.addListener = function (type, callback) {
  var newCallback

  callback._wrappedCallback = callback._wrappedCallback || []
  newCallback = wrap(callback, 'EventEmitter.on')
  if (newCallback !== callback) {
    callback._wrappedCallback.push(newCallback)
  }
  arguments[1] = newCallback
  return onEvent.apply(this, arguments)
}
var removeEvent = EventEmitter.prototype.removeListener
EventEmitter.prototype.removeListener = function (type, callback) {
  if (callback && Array.isArray(callback._wrappedCallback)) {
    arguments[1] = callback._wrappedCallback.pop()
  }
  return removeEvent.apply(this, arguments)
}

Basically, what I'm doing is still wrapping each callback, but I store the array on the original function so when it comes time to remove I can pop them off the array. This fixes the issue in http.js::onServerResponseClose.

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

2 participants