Skip to content

Change the timing to emit 'msg' event to consume a buffer in advance - #32

Closed
shinichy wants to merge 1 commit into
msgpack:masterfrom
silkedit:change_emit_msg_timing
Closed

shinichy wants to merge 1 commit into
msgpack:masterfrom
silkedit:change_emit_msg_timing

Conversation

@shinichy

Copy link
Copy Markdown

When I work with msgpack.Stream + deasync(https://github.com/abbr/deasync), the following program causes stack overflow.
This issue is caused because msgpack.Stream emits 'msg' event before it consumes a buffer and deasync allows processing io events before finishing 'msg' event callbacks.
I know this doesn't usually happen but it's better to emit 'msg' after consuming a buffer to handle this kind of case.

'use strict'

var net = require('net');
var path = '/tmp/deasync.sock';
var msgpack = require('msgpack');

var server = net.createServer(function(c) {
  c.on('data', function() {
    c.write(msgpack.pack('foo'));
  });
  c.on('end', function() {
    console.log('end')
    server.close()
  })
  c.write(msgpack.pack('hello'));
});
server.listen(path);

var done = false;
var client = net.createConnection(path)
var stream = new msgpack.Stream(client)
stream.on('msg', function(msg) {
  console.log('msg: ' + msg)
  if (msg === 'hello') {
    client.write(msgpack.pack('foo'));
    require('deasync').loopWhile(() => !done);
    console.log('done');
    client.end()
  } else if (msg === 'foo') {
    done = true;
  }
});

Output

msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
msg: hello
:0



RangeError: Maximum call stack size exceeded

newbee1905 added a commit to newbee1905/msgpack-node that referenced this pull request Jan 24, 2022
@enochgroot

Copy link
Copy Markdown
Collaborator

Superseded by 2.0.0 (#25687, e04c9b5). Stream now consumes the frame before emitting 'msg' (lib/msgpack.js), which is the change this PR proposed.

@enochgroot

Copy link
Copy Markdown
Collaborator

Re-evaluated on master e04c9b55. Stream already snapshots bytes_remaining and consumes the frame before emit('msg').

Repro, Node v20.20.2:

  • concatenated pack('a')+pack('b'), listener for 'a' called unpack([0x01]) → both frames delivered ["a","b"]
  • throw in the listener → emit count 1, no replay

Covered by delivers both frames when a msg listener calls unpack and does not re-emit a frame whose msg listener threw. Closing this PR as superseded by 2.0.0.

@enochgroot enochgroot closed this Sep 19, 2026
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.

2 participants