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

Avoid terminating connections prematurely #1511

Merged
merged 1 commit into from
Aug 6, 2015

Conversation

sorenisanerd
Copy link
Contributor

While sending very large messages (far beyond what fits in a the tcp
buffer, so it takes multiple sendto system calls for it to finish),
zmq_close will close the connection regardless of ZMQ_LINGER.

In case no engine is attached, a pipe->check_read() is needed to look
for the delimiter in the pipe and ultimately trigger the pipe
termination.

However, if there is an engine attached, the check_read() looks ahead
and finds the delimiter and terminates the connection even though the
engine might actually still be in the middle of sending a message.

This happens because while the io_thread is still busy sending the data,
the pipe can get terminated and the io thread ends up being terminated.

While sending very large messages (far beyond what fits in a the tcp
buffer, so it takes multiple sendto system calls for it to finish),
zmq_close will close the connection regardless of ZMQ_LINGER.

In case no engine is attached, a pipe->check_read() is needed to look
for the delimiter in the pipe and ultimately trigger the pipe
termination.

However, if there *is* an engine attached, the check_read() looks ahead
and finds the delimiter and terminates the connection even though the
engine might actually still be in the middle of sending a message.

This happens because while the io_thread is still busy sending the data,
the pipe can get terminated and the io thread ends up being terminated.
c-rack added a commit that referenced this pull request Aug 6, 2015
Avoid terminating connections prematurely
@c-rack c-rack merged commit 84a93d4 into zeromq:master Aug 6, 2015
@sorenisanerd
Copy link
Contributor Author

Thanks!

I would have left a comment on https://zeromq.jira.com/browse/LIBZMQ-554 and https://zeromq.jira.com/browse/LIBZMQ-551 asking them to test, but I can't seem to create an account on Jira. It says my domain is not allowed.

@xaqq
Copy link
Member

xaqq commented Aug 6, 2015

AFAIK Jira is not used anymore and all issue tracking happens on Github.

On Thu, Aug 6, 2015 at 8:54 PM, Soren Hansen notifications@github.com
wrote:

Thanks!

I would have left a comment on https://zeromq.jira.com/browse/LIBZMQ-554
and https://zeromq.jira.com/browse/LIBZMQ-551 asking them to test, but I
can't seem to create an account on Jira. It says my domain is not allowed.


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

Kapp Arnaud - Xaqq

@sorenisanerd
Copy link
Contributor Author

Oh, ok. Thanks!

@mikedevx
Copy link

Does anyone know the status of this fix, as it relates to a zmq release? Is it in a release, or not yet in a release?

I see this fix was committed Aug 6, 2015.
The most recent stable release 4.1.4, Dec 18, 2015, does not appear to contain this fix.
It appears to not be planned for inclusion in 4.1.5 either?

The company product I work on makes extensive use of ZMQ 3 Push Pull, sometimes in a connect-send-disconnect model where because of this bug messages are lost. We are VERY interested in upgrading to a version of ZMQ that contains this fix. Our workarounds for this bug are proving to be unreliable.

@hintjens
Copy link
Member

@mikedevx if it helps, we can push forwards with a 4.2 stable release.

@bluca
Copy link
Member

bluca commented Mar 30, 2016

@hintjens - should we wait to get ZMQ_REQ_CORRELATE fixed first, given it's marked as a stable API and it's broken at the moment? As far as I understand it's been looked at, see discussion in: #1730

@hintjens
Copy link
Member

Sure. It's also reasonable to make a release candidate with some pieces still needing work.

@mikedevx
Copy link

Thank you!
We are trying the 1511 fix as a patch on zmq v 3.2.3, and so far, so good in testing.
I would say, No need to rush 4.2. We’ll evaluate the upgrade path when it is released!

Thank you again!
Michael Devereaux

From: Pieter Hintjens [mailto:notifications@github.com]
Sent: Wednesday, March 30, 2016 6:59 AM
To: zeromq/libzmq
Cc: Michael Devereaux
Subject: Re: [zeromq/libzmq] Avoid terminating connections prematurely (#1511)

@mikedevxhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_mikedevx&d=CwMCaQ&c=8S5idjlO_n28Ko3lg6lskTMwneSC-WqZ5EBTEEvDlkg&r=KbV3rp4q2lvwTUd_oq0LWShyI451vfiOLjtzYiTyUso&m=r8Z2bVkQePtNBGjllCd9zlajuV6spFKA8Torp5MvM04&s=BivhrGCph1cr8EaQVp8vAjDjDtau9CHc7YF2-ka7HG4&e= if it helps, we can push forwards with a 4.2 stable release.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_zeromq_libzmq_pull_1511-23issuecomment-2D203396520&d=CwMCaQ&c=8S5idjlO_n28Ko3lg6lskTMwneSC-WqZ5EBTEEvDlkg&r=KbV3rp4q2lvwTUd_oq0LWShyI451vfiOLjtzYiTyUso&m=r8Z2bVkQePtNBGjllCd9zlajuV6spFKA8Torp5MvM04&s=KaaCGXl9Y1_mOOkiFEHzc8NVwm03k8ygRsurg6MKydM&e=


The information contained in this transmission may be confidential. Any disclosure, copying, or further distribution of confidential information is not permitted unless such privilege is explicitly granted in writing by Quantum. Quantum reserves the right to have electronic communications, including email and attachments, sent across its networks filtered through anti virus and spam software programs and retain such messages in order to comply with applicable data security and retention requirements. Quantum is not responsible for the proper and complete transmission of the substance of this communication or for any delay in its receipt.

@hintjens
Copy link
Member

Good stuff.
On 30 Mar 2016 17:45, "mikedevx" notifications@github.com wrote:

Thank you!
We are trying the 1511 fix as a patch on zmq v 3.2.3, and so far, so good
in testing.
I would say, No need to rush 4.2. We’ll evaluate the upgrade path when it
is released!

Thank you again!
Michael Devereaux

From: Pieter Hintjens [mailto:notifications@github.com]
Sent: Wednesday, March 30, 2016 6:59 AM
To: zeromq/libzmq
Cc: Michael Devereaux
Subject: Re: [zeromq/libzmq] Avoid terminating connections prematurely
(#1511)

@mikedevx<
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_mikedevx&d=CwMCaQ&c=8S5idjlO_n28Ko3lg6lskTMwneSC-WqZ5EBTEEvDlkg&r=KbV3rp4q2lvwTUd_oq0LWShyI451vfiOLjtzYiTyUso&m=r8Z2bVkQePtNBGjllCd9zlajuV6spFKA8Torp5MvM04&s=BivhrGCph1cr8EaQVp8vAjDjDtau9CHc7YF2-ka7HG4&e=>
if it helps, we can push forwards with a 4.2 stable release.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub<
https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_zeromq_libzmq_pull_1511-23issuecomment-2D203396520&d=CwMCaQ&c=8S5idjlO_n28Ko3lg6lskTMwneSC-WqZ5EBTEEvDlkg&r=KbV3rp4q2lvwTUd_oq0LWShyI451vfiOLjtzYiTyUso&m=r8Z2bVkQePtNBGjllCd9zlajuV6spFKA8Torp5MvM04&s=KaaCGXl9Y1_mOOkiFEHzc8NVwm03k8ygRsurg6MKydM&e=>


The information contained in this transmission may be confidential. Any
disclosure, copying, or further distribution of confidential information is
not permitted unless such privilege is explicitly granted in writing by
Quantum. Quantum reserves the right to have electronic communications,
including email and attachments, sent across its networks filtered through
anti virus and spam software programs and retain such messages in order to
comply with applicable data security and retention requirements. Quantum is
not responsible for the proper and complete transmission of the substance
of this communication or for any delay in its receipt.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1511 (comment)

@bluca
Copy link
Member

bluca commented Mar 30, 2016

@hintjens - What about backporting this fix to 4.0 and 4.1? Seems easy enough, I can do that if you want.

@hintjens
Copy link
Member

@bluca yes, nice. We have an issue, so if you could do that and update the NEWS in each case, that'd be awesome...

@bluca
Copy link
Member

bluca commented Mar 30, 2016

Sure! Is LIBZMQ-554 the issue? There was no mention of one in this PR

@hintjens
Copy link
Member

Ah, I was confusing #1511 with an issue. We need an explicit issue then.

@bluca
Copy link
Member

bluca commented Mar 30, 2016

Will open one.

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.

6 participants