Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Memory spike when emitting large objects without enforcing WS-only transports #2872

Closed
nicholaswmin opened this issue Feb 23, 2017 · 5 comments

Comments

@nicholaswmin
Copy link

nicholaswmin commented Feb 23, 2017

Current behaviour

  • When there's no enforcement to use "Web Sockets" only as the transport
    protocol, the server logs a memory spike (> 500%) when emitting a big sample
    JSON.

  • When "Web Sockets" only are enforced, the server logs
    nominal memory consumption values whilst emitting the sample JSON.

Steps to reproduce

I've created a repo illustrating this issue, https://github.com/TheProfs/socket-mem-leak

Expected behaviour

  • When there's no enforcement to use "Web Sockets" only as the transport
    protocol, the server logs a nominal memory increase due to buffering, equal to the size of the emitted JSON (~10MB)

Setup

  • OS: Mac OSX, Linux
  • socket.io version: 1.7.3

Heap Metrics

Without enforcing WebSockets-only

Listening on 3000, using default transport (websocket & xhr)
Used heap size 34.91 MB
new connection
emitting 10 MB payload
Used heap size 659.31 MB
Used heap size 659.50 MB
Used heap size 659.56 MB
Used heap size 659.56 MB
new connection
emitting 10 MB payload
Used heap size 593.52 MB
Used heap size 593.65 MB
Used heap size 593.68 MB
Used heap size 593.68 MB

Enforcing WebSockets-only

Listening on 3000, using WebSocket-only transport
Used heap size 34.92 MB
new connection
emitting 10 MB payload
Used heap size 54.68 MB
Used heap size 54.68 MB
Used heap size 54.68 MB
new connection
emitting 10 MB payload
Used heap size 43.93 MB
Used heap size 43.97 MB
Used heap size 43.98 MB
Used heap size 43.98 MB
@nicholaswmin nicholaswmin changed the title Memory Leak when emitting large objects without enforcing WS-only transports Memory leak when emitting large objects without enforcing WS-only transports Feb 23, 2017
@nicholaswmin
Copy link
Author

Tagging @hunterloftis from Heroku Tech Team - wanted to follow this issue

@darrachequesne
Copy link
Member

Thanks for the example, I could indeed reproduce the issue.

It seems the ucs2decode method here consumes a lot of memory. It's called there:

exports.encodePayload = function (packets, supportsBinary, callback) {
  if (typeof supportsBinary === 'function') {
    callback = supportsBinary;
    supportsBinary = null;
  }

  // <= supportsBinary is true here, so the payload is encoded as binary
  if (supportsBinary) {
    return exports.encodePayloadAsBinary(packets, callback);
  }

  if (!packets.length) {
    return callback('0:');
  }

  function encodeOne(packet, doneCallback) {
    exports.encodePacket(packet, supportsBinary, false, function(message) {
      doneCallback(null, setLengthHeader(message));
    });
  }

  map(packets, encodeOne, function(err, results) {
    return callback(results.join(''));
  });
};

@nicholaswmin
Copy link
Author

nicholaswmin commented Feb 25, 2017

I can dive in when things clear up and see if I can get a fix (in about a week or so) and perhaps submit a PR.

Unless you already have an idea on what's going on with ucs2decode

@nicholaswmin nicholaswmin changed the title Memory leak when emitting large objects without enforcing WS-only transports Memory spike when emitting large objects without enforcing WS-only transports Feb 26, 2017
@darrachequesne
Copy link
Member

It seems the behaviour was changed here: socketio/engine.io-parser@44c7aa5

@darrachequesne
Copy link
Member

Yet I'm not sure whether the statement the polling transport has to know the response type ahead of time is still valid.

darrachequesne added a commit to socketio/engine.io-parser that referenced this issue Mar 6, 2017
This reverts commit 44c7aa5, which caused string payloads to be encoded
as binary (so that huge string payloads were needlessly UTF-8-encoded).

Related: socketio/socket.io#2872
enderson-pan pushed a commit to holytiny/feathersjs-wxmp-socket.io-client that referenced this issue Nov 23, 2019
This reverts commit 44c7aa5, which caused string payloads to be encoded
as binary (so that huge string payloads were needlessly UTF-8-encoded).

Related: socketio/socket.io#2872
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants