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

Clean up ended streams to free leaked memory #200

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

argon
Copy link

@argon argon commented May 1, 2016

Fixes #64

When a stream ends remove references in _streamIds and _streamPriorities. This allows streams to be freed by the GC. (Verified with heap profiling)

@argon argon changed the title Clean up ended streams to free memory Clean up ended streams to free leaked memory May 1, 2016
@JakeChampion
Copy link

LGTM 👍

@gridnow
Copy link

gridnow commented May 30, 2016

I am testing this in pressure with following code by cmd:
node client.js 'https://localhost:8080/localhost.key':


var fs = require('fs');
var path = require('path');
var http2 = require('..');
var urlParse = require('url').parse;

// Setting the global logger (optional)
http2.globalAgent = new http2.Agent({
  rejectUnauthorized: true,
  log: require('../test/util').createLogger('client')
});

// Sending the request
var url = process.argv.pop();
var options = urlParse(url);

// Optionally verify self-signed certificates.
if (options.hostname == 'localhost') {
  options.key = fs.readFileSync(path.join(__dirname, '/localhost.key'));
  options.ca = fs.readFileSync(path.join(__dirname, '/localhost.crt'));
}

var r=0;
var finished = 0;
var max_req = 100;
var times = 10000;

function request() {
    var request = process.env.HTTP2_PLAIN ? http2.raw.get(options) : http2.get(options);
    // Receiving the response
    request.on('response', function(response) {
        response.on('data', function(data) {
        });
        response.on('end', finish);
    });
    request.on('socket', (socket) => {
        //console.log("socket event");
        //socket.emit('agentRemove');
    });
}

run_once();
// Quitting after both the response and the associated pushed resources have arrived

function run_once() {
    for (r = 0; r < max_req; r++) 
    {
        //console.log("Making request ", r);
        request();
    }
}

function finish() {
  //console.log("finished = ", finished);
  finished += 1;
  if (finished === max_req/*(1 + push_count)*/) {
    finished = 0;
    if (times-- === 0)
    {
        //process.exit();
        setInterval(function() {
            //idle for GC
//          global.gc();
            //console.log("end global agent: ", http2.globalAgent);

        }.bind(this), 1 * 1000);
    }
    else {
        run_once();
    }
    //global.gc();
    //run_once();
  }
}

but get error like this at server:(just from server.js of example)

13:36:45.274Z INFO server/http: New incoming HTTP/2 connection (e=97, client=::ffff:127.0.0.1:62351, SNI=localhost)
13:36:45.275Z INFO server/http: New incoming HTTP/2 connection (e=98, client=::ffff:127.0.0.1:62352, SNI=localhost)
13:36:45.277Z INFO server/http: New incoming HTTP/2 connection (e=99, client=::ffff:127.0.0.1:62353, SNI=localhost)
13:36:45.311Z ERROR server/connection: Invalid incoming stream ID. (e=0, stream_id=1, lastIncomingStream=199)
13:36:45.311Z FATAL server/endpoint: Fatal error, closing connection (e=0, source=connection, message=PROTOCOL_ERROR)
13:36:45.312Z ERROR server/stream: Received illegal frame. (e=0, s=100, error=PROTOCOL_ERROR, state=IDLE)
frame: {
"id": 0,
"type": "WINDOW_UPDATE",
"flags": [],
"stream": 1,
"window_size": 887
}
^[[A^[[A

@gridnow
Copy link

gridnow commented May 30, 2016

Original code with huge memory leak!

@argon
Copy link
Author

argon commented Sep 13, 2016

Any updates on this?

@ide
Copy link

ide commented Sep 22, 2016

cc @nwgh

@AndrewBarba
Copy link

Can we please get an update on merging this? It's been months

florianreinhart and others added 3 commits November 8, 2016 10:56
latest http2 including mem leak fix and stream.upstream not defined check
Fixes molnarg#228

In the case where this._push(frame) returns null (i.e., the frame is too
large for the window and split or the window size is <=0), moreNeeded
will be set to null. Then this._queue.push(frame) is called, but
moreNeeded is still null. Thus, any time the window is <=0 or the frame
is split we'll hit the assert:

  var moreNeeded = null;
  if (this._queue.length === 0) {
    moreNeeded = this._push(frame);
  }

  if (moreNeeded === null) {
    this._queue.push(frame);
  }

  return moreNeeded;

Credit goes to @jrabek for original version of this patch
Additional debug data is allowed to be included in the GOAWAY frame:
https://http2.github.io/http2-spec/#GOAWAY. We now put that data into
frame.debug_data instead of returning a FRAME_SIZE_ERROR. Fixes molnarg#218
and molnarg#219.
@ShaharHD
Copy link

@AndrewBarba I think none of the maintainers are active ... the last one to commit anything was @nwgh and that was in September 2016 ...

@hthetiot
Copy link

merged https://github.com/kaazing/node-http2/commit/db0936244be639cbfd9a45c85377d0923a123b46

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

Successfully merging this pull request may close these issues.

10 participants