Fix #84 - slice outgoing messages according to frameMax #127

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@majek
majek commented Sep 18, 2012

Content frames are ignoring the frame max setting, and just send the entire body as one frame. (see #84)

This commit does three things:

  1. amends maxFrameBuffer to the value advertised by the server
  2. extracts main logic from _sendBody function to a _bodyToBuffer
  3. _sendBody is rewritten in a simpler fashion, assuming it only deals with buffers (non-buffers are being converted to a buffer in _bodyToBuffer)

I don't know how to run the tests from test directory, I verified this code manually.

@postwait postwait commented on the diff Sep 18, 2012
@@ -983,6 +983,10 @@ Connection.prototype._onMethod = function (channel, method, args) {
// 4. The server responds with a connectionTune request
case methods.connectionTune:
+ if (args.frameMax) {
+ debug("tweaking maxFrameBuffer to " + args.frameMax);
+ maxFrameBuffer = args.frameMax;
+ }
postwait
postwait Sep 18, 2012 Owner

Please split this change into its own commit and pull request.

majek
majek Sep 26, 2012

The whole point of this pull request is to adhere to frameMax advertised by the server. Additionally frameMax advertised by the server by default is equal to the predefined maxFrameBuffer (ie: 128KiB), so this line is effectively NOP for rabbit with default settings.

Owner

Have you benchmarked the code there to make sure you had no regressions in performance?

majek commented Sep 26, 2012

Simple benchmark: https://gist.github.com/3787602

On default (80fc049):
$ time node t.js 1 > 0.408s
$ time node t.js 2 > 0.348s
$ time node t.js 3 > 0.349s

On my branch (1b322a5) + hardcoded max frame size for outgoing messages to 1024 bytes (ie: very small frame):
$ time node t.js 1 > 0.458s
$ time node t.js 2 > 0.391s
$ time node t.js 3 > 0.395s

I can't benchmark it for larger messages / larger number of messages because node-amqp breaks when the outgoing buffer is large (for my benchmark - it looks like all messages are being sent to meta queue rather than test)

Owner
postwait commented Jan 8, 2013

merged

@postwait postwait closed this Jan 8, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment