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

Saner connection option defaults #83

Closed
wants to merge 2 commits into from

Conversation

carlhoerberg
Copy link
Contributor

channelMax: 65535 channels
frameMax: 131072 bytes
heartbeat: 580s

@michaelklishin
Copy link

Some background: RabbitMQ 3.3.5 will enforce server-side configured channel_max. "Unlimited" has always been a questionable default and getting an error message in the client would be better than having RabbitMQ close TCP connection.

As for heartbeats, the value of 580 was chosen because it works well with F5 load balancers: we get a frame sent just under every 5 minutes.

@squaremo
Copy link
Collaborator

squaremo commented Aug 3, 2014

Channel max: 65535 is the maximum anyway, since it's a 16-bit uint. It could be better enforced by the client perhaps.

Heartbeat: It's difficult to justify giving this a default value other than zero, while zero is also a poor choice. Is it more surprising to fail a heartbeat without having set a value, or not to detect a connection failure (for an extended period)? Not sure.

Frame max: you don't provide a motivation. Having a small number -- in fact, the smallest allowed -- seems like a good idea to me, since it will fragment large messages and let channels be multiplexed more effectively. I can't think of any reason to have a really large frame max, and specifically not that number (surely Qpid can't still have that bug?)

@carlhoerberg
Copy link
Contributor Author

Channel max: If the server limits channel max and your client tries with 0 channels, your code will try to tune to 0 as it's "lower" than the server's channel max, but the server will then disconnect the client as it isn't respecting the channelMax. If the client instead defaults to uint16.max it will negotiate the lowest of the two values, doing the right thing.

Heartbeats: Without heartbeats connections can stay open for hours (holding unacked messages, using resources etc). Lost heartbeats are (in out experience at @cloudamqp) very rare. Dead connections on the other hand gives users tons of headache.

Frame max: I'm not sure, 128KB is the default for practically all other clients. I guess it's a trade off, 128 KB is not much on a LAN connection, would the 4KB instead give more jitter? More context switching for the server? In our experience few messages are larger than 4KB anyway so I guess it doesn't really matter..

On Sunday 3 August 2014 at 13:07, Michael Bridgen wrote:

Channel max: 65535 is the maximum anyway, since it's a 16-bit uint. It could be better enforced by the client perhaps.
Heartbeat: It's difficult to justify giving this a default value other than zero, while zero is also a poor choice. Is it more surprising to fail a heartbeat without having set a value, or not to detect a connection failure (for an extended period)? Not sure.
Frame max: you don't provide a motivation. Having a small number -- in fact, the smallest allowed -- seems like a good idea to me, since it will fragment large messages and let channels be multiplexed more effectively. I can't think of any reason to have a really large frame max, and specifically not that number (surely Qpid can't still have that bug?)


Reply to this email directly or view it on GitHub (#83 (comment)).

@michaelklishin
Copy link

I'm not sure I understand how the smallest possible frame_max will improve efficiency. I expect it to be lower with smaller values (you will have to frame more often than not), there is no padding if the size of the payload is lower than frame_max.

As for heartbeats, not only TCP timeouts can be really high, intermediates like load balancers are known to close low activity connections aggressively. After switching RabbitMQ to 580 seconds by default we see a lot fewer reports of issues that boil down to "our load balancer closes TCP connections to RabbitMQ because of periods of inactivity".

@rade
Copy link

rade commented Aug 3, 2014

I do not understand the point of these changes. Clients are meant to have the following tuning logic:

  1. when server sends 0, reply with the configured client value
  2. otherwise, when the configured client value is 0, reply with sent server value
  3. otherwise, reply with min of client and server value

With that logic in place, the clients default values can be 'unlimited', since that simply means the server values will take effect.

@rade
Copy link

rade commented Aug 3, 2014

I'd also like to point out that, contrary to what michaelklishin said above, the java client sets the defaults to 'unlimited' for everything. It's the server default which got changed to 580 a while ago.

@carlhoerberg
Copy link
Contributor Author

@rade That's not what the code is saying: https://github.com/squaremo/amqp.node/blob/master/lib/connection.js#L188
if the server's value is 0 then use the client's value, if it's not then use the lowest of the two, and as the client is specifying 0 then use that.. So I guess a change there is required too, or only.

@squaremo
Copy link
Collaborator

squaremo commented Aug 3, 2014

I'm not sure I understand how the smallest possible frame_max will improve efficiency.

I didn't say it would. I don't think it hurts efficiency especially, on the other hand, and it does improve fairness (one channel can't hog the socket). I admit this is unscientific -- as is, I rather suspect, the setting for all client libraries to date.

Channel max: If the server limits channel max and your client tries with 0 channels, your code will try to tune to 0 as it's "lower" than the server's channel max, but the server will then disconnect the client as it isn't respecting the channelMax.

If there's a bug in the negotiation of these values, I should fix that, yep. That's different from changing the default values, though.

@squaremo
Copy link
Collaborator

squaremo commented Aug 3, 2014

I do not understand the point of these changes. Clients are meant to have the following tuning logic:

  1. when server sends 0, reply with the configured client value
  2. otherwise, when the configured client value is 0, reply with sent server value
  3. otherwise, reply with min of client and server value

@rade Does that mean my comment in the code is wrong (specifically the last sentence)?

// We get sent values for channelMax, frameMax and heartbeat,
// which we may accept or lower (subject to a minimum for
// frameMax, but we'll leave that to the server to enforce). In
// all cases, `0` really means `+infinity`, that is, no limit
// (except that of the encoded representation, e.g., unsigned
// short for channelMax). RabbitMQ allows all these figures to be
// negotiated downward, *including* to zero i.e., no limit.

@rade
Copy link

rade commented Aug 3, 2014

@squaremo yep, that's wrong.

@squaremo
Copy link
Collaborator

squaremo commented Aug 3, 2014

@squaremo yep, that's wrong.

Ops! So it seems like the thing to do is fix the negotiation. I am also OK with setting a sensible heartbeat (just < 5min seems OK). Channel max is fine as 0, so long as the negotiation works properly, and it doesn't seem like there's any clear motivation for changing frame max.

@rade
Copy link

rade commented Aug 3, 2014

no need to set a non-zero heartbeat, since the server default will take care of that.

@carlhoerberg
Copy link
Contributor Author

That's true, no need for heartbeat, if the negotiation function is correct, but channelMax is still a problem, because both the server and this client will suggest 0, but it looks like this line of code https://github.com/squaremo/amqp.node/blob/master/lib/connection.js#L423 then could overflow 2^16. RabbitMQ server might change the default to 2^16-1 but it's not yet certain as of when.

channelMax: 65535 channels
frameMax: 131072 bytes
heartbeat: 580s
@squaremo
Copy link
Collaborator

but it looks like this line of code https://github.com/squaremo/amqp.node/blob/master/lib/connection.js#L423 then could overflow 2^16

Later in the connection open epic any "unlimited" values are given their effective maximums according to the fields' encodings: https://github.com/squaremo/amqp.node/blob/master/lib/connection.js#L234

@carlhoerberg
Copy link
Contributor Author

yes, realized later that 0 || 1 == 1 in javascript

@squaremo
Copy link
Collaborator

That takes care of everything I believe.

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.

4 participants