Bug 894375 - --tcp_nodelay broker option to have yes/no values
--tcp_nodelay broker option to have yes/no values
Status: CLOSED ERRATA
Product: Red Hat Enterprise MRG
Classification: Red Hat
Component: qpid-cpp (Show other bugs)
2.2
All All
medium Severity low
: 3.0
: ---
Assigned To: Andrew Stitcher
Ernie
: EasyFix, Patch
Depends On:
Blocks: 768114 961008 961009
  Show dependency treegraph
 
Reported: 2013-01-11 10:33 EST by Pavel Moravec
Modified: 2014-09-24 11:05 EDT (History)
5 users (show)

See Also:
Fixed In Version: qpid-cpp-0.22-8
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 961009 (view as bug list)
Environment:
Last Closed: 2014-09-24 11:05:08 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Proposed patch (1.17 KB, patch)
2013-01-11 10:36 EST, Pavel Moravec
no flags Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
Apache JIRA QPID-3689 None None None Never

  None (edit)
Description Pavel Moravec 2013-01-11 10:33:55 EST
Description of problem:
Currently it is only possible to disable Naggle's algorithm by setting --tcp_nodelay option. However that option can be only enabled and current behavior is confusing (see https://bugzilla.redhat.com/show_bug.cgi?id=768114#c1). And mainly by fixing https://bugzilla.redhat.com/show_bug.cgi?id=768114, we need both possibilities to enable and also to disable the option.


Version-Release number of selected component (if applicable):
0.14-22 (also in 0.18-13)


How reproducible:
100%


Steps to Reproduce:
n.a., see Expected results for proper/proposed behavior

  
Actual results:
No option to disable tcp_nodelay (though it is disabled by default).


Expected results:
Run qpidd with parameters below and every time let a client to connect (any client, even auth.failure is enough).
# qpidd --trace --tcp-nodelay=yes 2>&1 | grep TCP_NODELAY
2013-01-11 15:21:53 info Set TCP_NODELAY on connection to 127.0.0.1:49356
^C
# qpidd --trace --tcp-nodelay yes 2>&1 | grep TCP_NODELAY
2013-01-11 15:22:04 info Set TCP_NODELAY on connection to 127.0.0.1:49357
^C
# qpidd --trace --tcp-nodelay no 2>&1 | grep TCP_NODELAY
^C
# qpidd --trace --tcp-nodelay false 2>&1 | grep TCP_NODELAY
^C
# qpidd --trace --tcp-nodelay true 2>&1 | grep TCP_NODELAY
2013-01-11 15:22:54 info Set TCP_NODELAY on connection to 127.0.0.1:49360
^C
# qpidd --trace --tcp-nodelay false 2>&1 | grep TCP_NODELAY
^C
# qpidd --trace --tcp-nodelay 
2013-01-11 15:22:41 debug Exception constructed: Error in command line options: required parameter is missing in 'tcp-nodelay'
Use --help to see valid options

2013-01-11 15:22:41 critical Unexpected error: Error in command line options: required parameter is missing in 'tcp-nodelay'
Use --help to see valid options

#

That is:
--tcp-nodelay{=| }{yes|true} should set TCP_NODELAY on connections
--tcp-nodelay{=| }{no|false} should unset it
--tcp-nodelay (without arguments) is invalid syntax (here we loose backward compatibility, though)


Additional info:
Trivial patch provided, already tested.
Comment 1 Pavel Moravec 2013-01-11 10:36:33 EST
Created attachment 676927 [details]
Proposed patch
Comment 4 Justin Ross 2013-04-10 17:56:48 EDT
I think a sensible pattern is to adopt -enabled as a suffix for long options that take booleans.  This has the virtue of fixing the misfeature of program options and getopt that allows you to shorten long options, causing ambiguities (ugh!).

But before we get there, I think I've been a little confused about this one. We can support a default no-arg behavior for --tcp-nodelay, true?  That is:

  --tcp-nodelay no  --> disabled
  --tcp-nodelay yes --> enabled
  --tcp-nodelay     --> our code default (which would change to enabled)

Justin
Comment 5 Andrew Stitcher 2013-04-11 13:02:07 EDT
(In reply to comment #4)
> I think a sensible pattern is to adopt -enabled as a suffix for long options
> that take booleans.  This has the virtue of fixing the misfeature of program
> options and getopt that allows you to shorten long options, causing
> ambiguities (ugh!).
> 
> But before we get there, I think I've been a little confused about this one.
> We can support a default no-arg behavior for --tcp-nodelay, true?  That is:
> 
>   --tcp-nodelay no  --> disabled
>   --tcp-nodelay yes --> enabled
>   --tcp-nodelay     --> our code default (which would change to enabled)
> 

I think I have a workable variant of this:

We introduce a new option --tcp-nodelay-enable (we are already using "-enable" as a suffix in a couple of places, but not using "-enabled"). 

so we have 3 possible options now:
  --tcp-nodelay-enable no
  --tcp-nodelay-enable yes
  --tcp-nodelay

BUT the first 2 can be abbreviated to --tcp-nodelay no & --tcp-nodelay yes; So I think we have the result we are looking for.
Comment 6 Andrew Stitcher 2013-04-15 11:28:28 EDT
(In reply to comment #5)
> (In reply to comment #4)
> > I think a sensible pattern is to adopt -enabled as a suffix for long options
> > that take booleans.  This has the virtue of fixing the misfeature of program
> > options and getopt that allows you to shorten long options, causing
> > ambiguities (ugh!).
> > 
> > But before we get there, I think I've been a little confused about this one.
> > We can support a default no-arg behavior for --tcp-nodelay, true?  That is:
> > 
> >   --tcp-nodelay no  --> disabled
> >   --tcp-nodelay yes --> enabled
> >   --tcp-nodelay     --> our code default (which would change to enabled)
> > 
> 
> I think I have a workable variant of this:
> 
> We introduce a new option --tcp-nodelay-enable (we are already using
> "-enable" as a suffix in a couple of places, but not using "-enabled"). 
> 
> so we have 3 possible options now:
>   --tcp-nodelay-enable no
>   --tcp-nodelay-enable yes
>   --tcp-nodelay
> 
> BUT the first 2 can be abbreviated to --tcp-nodelay no & --tcp-nodelay yes;
> So I think we have the result we are looking for.

Unfortunately the abbreviation doesn't work in the current code base.

Moreso, the non command line configuration options - config file and environment variables work "correctly" as is and you are forced to use tcp-nodelay=yes|no in the config file and QPID_TCP_NODELAY=yes|no as an environment variable so they don't need a new name at all.
Comment 7 Andrew Stitcher 2013-04-17 19:03:46 EDT
The change to resolve this issue has now gone into the qpid code base on trunk for 0.23 in r1469088.

The following should be the basis of documentation/release note.
For RHEL 6 and later:
Note that this introduces the possibility of command line options
--tcp-nodelay=yes & --tcp-nodelay=no (and all the other ways of doing similar)

For RHEL5:
It was not possible to easily add optional arguments to command line options because it is not supported by the boost library used (1.33).

As the default is now equivalent to always specifying --tcp-nodelay it is necessary to use the config file or an environment variable to turn the option off if that is desired on RHEL5.
Comment 8 Andrew Stitcher 2013-04-18 13:19:41 EDT
A necessary fix to the original change has gone in to r1469466 on qpid trunk
Comment 9 Ernie 2013-07-31 13:32:42 EDT
Summary: Failed to disable to the TCP_NODELAY option using the config file. All other methods succeed (command line and environment variable)

For rhel 6.4 32/64
Verified for the command line options 
  --tcp-nodelay no
  --tcp-nodelay off
  --tcp-nodelay 0
  --tcp-nodelay false

  --tcp-nodelay
  --tcp-nodelay yes
  --tcp-nodelay on
  --tcp-nodelay 1
  --tcp-nodelay true
  If no --tcp-nodelay argument then TCP_NODELAY is enabled
  If --tcp-nodelay is set to junk (--tcp-nodelay=blah), then broker doesn't start.

Verified for environment variable
  QPID_TCP_NODELAY=no
  QPID_TCP_NODELAY=off
  QPID_TCP_NODELAY=0
  QPID_TCP_NODELAY=false

  QPID_TCP_NODELAY=
  QPID_TCP_NODELAY=yes
  QPID_TCP_NODELAY=on
  QPID_TCP_NODELAY=1
  QPID_TCP_NODELAY=true
  If QPID_TCP_NODELAY is set to junk (blah), then broker doesn't start.

Verified for config file
  No entry in config file results in TCP_NODELAY being enabled

FAILED for config file
  tcp-nodelay=no
  tcp-nodelay=off
  tcp-nodelay=0
  tcp-nodelay=junk

Basically, the broker will start with a tcp-nodelay=<anything> in the config file, but TCP_NODELAY is always enabled. There appears to be no way to disable TCP_NODELAY using the config file. Based on comment 7, disabling this option using the config file should be supported.

Package list from rhel 6.4 64
------------------------------
cyrus-sasl-2.1.23-13.el6_3.1.x86_64
cyrus-sasl-devel-2.1.23-13.el6_3.1.x86_64
cyrus-sasl-gssapi-2.1.23-13.el6_3.1.x86_64
cyrus-sasl-lib-2.1.23-13.el6_3.1.x86_64
cyrus-sasl-md5-2.1.23-13.el6_3.1.x86_64
cyrus-sasl-plain-2.1.23-13.el6_3.1.x86_64
python-qpid-0.22-4.el6.noarch
python-qpid-qmf-0.22-7.el6.x86_64
python-saslwrapper-0.22-3.el6.x86_64
qpid-cpp-client-0.22-8.el6.x86_64
qpid-cpp-client-devel-0.22-8.el6.x86_64
qpid-cpp-client-devel-docs-0.22-8.el6.noarch
qpid-cpp-client-rdma-0.22-8.el6.x86_64
qpid-cpp-client-ssl-0.22-8.el6.x86_64
qpid-cpp-debuginfo-0.22-8.el6.x86_64
qpid-cpp-server-0.22-8.el6.x86_64
qpid-cpp-server-devel-0.22-8.el6.x86_64
qpid-cpp-server-ha-0.22-8.el6.x86_64
qpid-cpp-server-rdma-0.22-8.el6.x86_64
qpid-cpp-server-ssl-0.22-8.el6.x86_64
qpid-cpp-server-store-0.22-8.el6.x86_64
qpid-cpp-server-xml-0.22-8.el6.x86_64
qpid-cpp-tar-0.22-8.el6.noarch
qpid-java-client-0.22-5.el6.noarch
qpid-java-common-0.22-5.el6.noarch
qpid-java-example-0.22-5.el6.noarch
qpid-proton-c-0.4-2.2.el6.x86_64
qpid-proton-c-devel-0.4-2.2.el6.x86_64
qpid-proton-debuginfo-0.4-2.2.el6.x86_64
qpid-qmf-0.22-7.el6.x86_64
qpid-qmf-debuginfo-0.22-7.el6.x86_64
qpid-qmf-devel-0.22-7.el6.x86_64
qpid-snmpd-1.0.0-12.el6.x86_64
qpid-snmpd-debuginfo-1.0.0-12.el6.x86_64
qpid-tests-0.22-4.el6.noarch
qpid-tools-0.22-3.el6.noarch
saslwrapper-0.22-3.el6.x86_64
saslwrapper-devel-0.22-3.el6.x86_64


rhel 5 was not tested.
Comment 10 Andrew Stitcher 2013-07-31 16:34:18 EDT
Are you sure that you were modifying the correct config file? the location has changed I believe it should now be in /etc/qpid/qpidd.conf.
Comment 11 Ernie 2013-08-01 14:00:46 EDT
Yes :) I was starting the broker with a local config file (using the --config argument). But just to be sure it was actually reading the config file, I added a "string=foo" line to the config file and tried to start the broker. It failed with an error parsing the config file.

On a related note, The broker was able to start when the config file contained 
tcp-nodelay=random-junk
It started with TCP_NODELAY enabled, but I suspect that is only because it is now enabled by default. 
The command line and environment variable parser complains when it can't parse the value into a boolean, but the config file parser doesn't.
Comment 12 Andrew Stitcher 2013-09-10 16:16:36 EDT
Data point:

Disabling tcp-nodelay works as expected using Fedora 19 (gcc 4.8.1 & boost 1.53) and trunk qpid.
Comment 16 Ernie 2013-09-11 07:46:13 EDT
Verified

Retested with latest packages. tcp-nodelay is working as expected from the command line, environment and config file.

The environment that failed was the 0.22-8 packages. I re-installed these and saw the failure when using the config file. However, when tested against the 0.22-13 packages, setting tcp-nodelay with the config file now works.

Passed with packages
------------------
cyrus-sasl-2.1.23-13.el6_3.1.x86_64
cyrus-sasl-devel-2.1.23-13.el6_3.1.x86_64
cyrus-sasl-gssapi-2.1.23-13.el6_3.1.x86_64
cyrus-sasl-lib-2.1.23-13.el6_3.1.x86_64
cyrus-sasl-md5-2.1.23-13.el6_3.1.x86_64
cyrus-sasl-plain-2.1.23-13.el6_3.1.x86_64
perl-qpid-0.22-5.el6.x86_64
python-qpid-0.22-4.el6.noarch
python-qpid-qmf-0.22-10.el6.x86_64
python-saslwrapper-0.22-3.el6.x86_64
qpid-cpp-client-0.22-13.el6.x86_64
qpid-cpp-client-devel-0.22-13.el6.x86_64
qpid-cpp-client-devel-docs-0.22-13.el6.noarch
qpid-cpp-client-rdma-0.22-13.el6.x86_64
qpid-cpp-client-ssl-0.22-13.el6.x86_64
qpid-cpp-debuginfo-0.22-13.el6.x86_64
qpid-cpp-server-0.22-13.el6.x86_64
qpid-cpp-server-devel-0.22-13.el6.x86_64
qpid-cpp-server-ha-0.22-13.el6.x86_64
qpid-cpp-server-rdma-0.22-13.el6.x86_64
qpid-cpp-server-ssl-0.22-13.el6.x86_64
qpid-cpp-server-store-0.22-13.el6.x86_64
qpid-cpp-server-xml-0.22-13.el6.x86_64
qpid-cpp-tar-0.22-13.el6.noarch
qpid-java-client-0.22-5.el6.noarch
qpid-java-common-0.22-5.el6.noarch
qpid-java-example-0.22-5.el6.noarch
qpid-proton-c-0.5-2.el6.x86_64
qpid-proton-c-devel-0.5-2.el6.x86_64
qpid-proton-debuginfo-0.5-2.el6.x86_64
qpid-qmf-0.22-10.el6.x86_64
qpid-qmf-debuginfo-0.22-10.el6.x86_64
qpid-qmf-devel-0.22-10.el6.x86_64
qpid-snmpd-1.0.0-12.el6.x86_64
qpid-snmpd-debuginfo-1.0.0-12.el6.x86_64
qpid-tests-0.22-4.el6.noarch
qpid-tools-0.22-3.el6.noarch
rh-qpid-cpp-tests-0.22-13.el6.x86_64
ruby-qpid-0.7.946106-2.el6.x86_64
saslwrapper-0.22-3.el6.x86_64
saslwrapper-devel-0.22-3.el6.x86_64
Comment 17 errata-xmlrpc 2014-09-24 11:05:08 EDT
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHEA-2014-1296.html

Note You need to log in before you can comment on or make changes to this bug.