Problem: REP leaves label msgs for dead REQ in pipe

[bluca]

Solution: roll back the pipe if writing messages other than the
first fails in router::xsend.
Also add test case that reproduces the memory leak when ran with
valgrind.
Fixes #2567

[bluca]

@somdoron I think this is the right thing to do, but I could be missing something, could you please have a quick look when you have time?

[somdoron]

I think the fix is incorrect, take a look at the pipe terminated method, current out will be null and all msg will be dropped (future and past)

[bluca]

Thanks, will look again

[bjovke]

@somdoron current_out = NULL; happens after current_out->rollback (); with this fix anyway.
The issue could be only that rollback() removes all messages from pipe, I really don't know if this is correct behavior?
"all msg will be dropped (future and past)" current_out = NULL; existed before this patch. What exactly do you mean by this?

[somdoron]

Ok Sorry, I'm surprised we only find out about this now.
We also need to call the rollback in the xpipe_terminated (or only there and remove the current_out=null from the xsend, letting the xpipe_termianted handle it)

Also calling rollback in the router version is a mistake because it set the more_out to false, which is not what we want here (because it is a silent drop, setting more_out to false will cause an error on the next part, which the router will think is a routing id).

So I would just call outpipe.rollback() in both places.

[bjovke]

What I've discovered in my test:

• When message is received by ZMQ_REP server, zmq::stream_engine_t::in_event () is called, which eventually calls zmq::metadata_t::add_ref () for metadata member.
• ref_cnt of metadata grows to 4.
• Client is killed before server tries to respond.
• ref_cnt of metadata is decreased to 1 after whole cleanup has ended and metadata is never freed.
• With patch from @bluca ref_cnt correctly decreases to 0 and LIBZMQ_DELETE(metadata); is called in zmq::stream_engine_t destructor.
• This correctly corresponds to something left referencing metadata, probably one message which is not destroyed.

[bjovke]

I'm also not sure why there's current_out = NULL; in zmq::router_t::xsend, zmq::router_t::rollback and zmq::router_t::xpipe_terminated both set this to NULL in a more appropriate way.

[bluca]

Thanks for the comments&insights

As far as I can see, setting current_out to NULL in xsend has been around since forever, I can trace it back to before router.cpp existed, in xrep: b79d07b

Also calling rollback in the router version is a mistake because it set the more_out to false, which is not what we want here (because it is a silent drop, setting more_out to false will cause an error on the next part, which the router will think is a routing id).

Note that I intentionally called outpipe's rollback rather than router for this reason, already in this diff.

So if I followed correctly, the proposal is to resend this same PR, with the addition of calling pipe_->rollback() in xpipe_terminated ?

[somdoron]

Yes On May 17, 2017 12:23 AM, "Luca Boccassi" <notifications@github.com> wrote: Thanks for the comments&insights As far as I can see, setting current_out to NULL in xsend has been around since forever, I can trace it back to before router.cpp existed, in xrep: b79d07b <b79d07b> Also calling rollback in the router version is a mistake because it set the more_out to false, which is not what we want here (because it is a silent drop, setting more_out to false will cause an error on the next part, which the router will think is a routing id). Note that I intentionally called outpipe's rollback rather than router for this reason, already in this diff. So if I followed correctly, the proposal is to resend this same PR, with the addition of calling pipe_->rollback() in xpipe_terminated ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2572 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AClv9n6U_bUAOEu5mzT9cEPdnKvM_pMqks5r6hPmgaJpZM4NXVKd> .

[bluca]

Here it is: #2577