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

Cleaning up recent option names #1786

Merged
merged 5 commits into from
Feb 9, 2016
Merged

Cleaning up recent option names #1786

merged 5 commits into from
Feb 9, 2016

Conversation

hintjens
Copy link
Member

@hintjens hintjens commented Feb 9, 2016

@minrk I've also noticed the large message test freezes my laptop for a few seconds. Since we're now implementing a maximum message size, I've made this configurable via zmq_ctx_set(). This was long overdue I think. Needs a little help to be fully working.

Solution: rename to ZMQ_MAXRT

This is the option name used on Windows, so easier to use and
remember.
This option has a few issues. The name is long and clumsy. The
functonality is not smooth: one must set both this and
ZMQ_XPUB_VERBOSE at the same time, or things will break mysteriously.

Solution: rename to ZMQ_XPUB_VERBOSER and make an atomic option.

That is, implicitly does ZMQ_XPUB_VERBOSE.
The proper name is ZMQ_THREAD_SAFE

Solution: fix in the documentation.
These options are confusing and redundant. Their names suggest
they apply to the tcp:// transport, yet they are used for all
stream protocols. The methods zmq::set_tcp_receive_buffer and
zmq::set_tcp_send_buffer don't use these values at all, they use
ZMQ_SNDBUF and ZMQ_RCVBUF.

Solution: merge these new options into ZMQ_SNDBUF and ZMQ_RCVBUF.

This means defaulting these two options to 8192, and removing the
new options. We now have ZMQ_SNDBUF and ZMQ_RCVBUF being used both
for TCP socket control, and for input/output buffering.

Note: the default for SNDBUF and RCVBUF are otherwise 4096.
And I'm on a reasonably sized laptop. I think allocating INT_MAX
memory is dangerous in a test case.

Solution: expose this as a context option. I've used ZMQ_MAX_MSGSZ
and documented it and implemented the API. However I don't know how
to get the parent context for a socket, so the code in zmq.cpp is
still unfinished.
@minrk
Copy link
Member

minrk commented Feb 9, 2016

This defines (but does not implement) a "maximum" max msg size as INT_MAX. Shouldn't that be closer to SIZE_MAX, instead?

This seems strange to me, because larger messages do work, providing zmq_msg methods are used. I made my patch because messages larger than this were working fine, but reporting a false failure. This would make it impossible to ever work, I think.

@hintjens
Copy link
Member Author

hintjens commented Feb 9, 2016

OK, so this change is unfinished. The s_sendmsg truncates to INT_MAX, so that is the default I used for MAX_MSGSZ. There seems no point defaulting to a larger (>2Gb) size.

Why do you think it would never work?

@minrk
Copy link
Member

minrk commented Feb 9, 2016

Why do you think it would never work?

INT_MAX seems a fine default, but the docs added here state that it's not just the default, it's also the max allowed value:

The 'ZMQ_MAX_MSGSZ' argument sets the maximum allowed size
of a message sent in the context.

Default value:: INT_MAX
Maximum value:: INT_MAX

I read that as:

  • attempting to send a message larger than ZMQ_MAX_MSGSZ will fail
  • ZMQ_MAX_MSGSZ may not be larger than INT_MAX

Therefore:

  • messages may not ever be larger than INT_MAX

Is that wrong? Are you only meaning to set the truncation for rc, but not actually checking and failing in send/recv?

minrk added a commit that referenced this pull request Feb 9, 2016
Cleaning up recent option names
@minrk minrk merged commit 42ab88e into zeromq:master Feb 9, 2016
@hintjens
Copy link
Member Author

hintjens commented Feb 9, 2016

You're right, the code isn't correct. I misread it. So I want to change the problem statement to "unlimited message size is an insecure default." In that case a realistic max size is indeed SIZE_MAX.

I need someone to help me access the parent context from a socket_base and then I can finish this...

Thanks for the explanations.

@minrk
Copy link
Member

minrk commented Feb 11, 2016

I noticed that there is already a ZMQ_MAXMSGSIZE socket option, which is applied to receiving messages. Perhaps that should be extended to sends, rather than adding a new context option.

@hintjens
Copy link
Member Author

Good catch. This is a socket option, whereas I'd started making a context
option.

OK, I'll scratch my patch and look at using the socket ZMQ_MAXMSGSIZE.

On Thu, Feb 11, 2016 at 11:21 AM, Min RK notifications@github.com wrote:

I noticed that there is already a ZMQ_MAXMSGSIZE socket option, which is
applied to receiving messages. Perhaps that should be extended to sends,
rather than adding a new context option.


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

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