[lttng-dev] [PATCH lttng-tools v2] Fix: wrong use of the relay_streams_sent in snapshot

[Mathieu Desnoyers]

----- On Aug 25, 2017, at 12:47 PM, Julien Desfossez jdesfossez at efficios.com wrote:

>>>> If you remove this, I think the streams will never get published
>>>> within the relayd. (publish_connection_local_streams()). Is this
>>>> an expected side-effect ? It should be documented in the changelog.
>>>> My guess is that we indeed don't want to publish the snapshot
>>>> streams to the viewers.
>>> Indeed, a snapshot session cannot be a live session, so we don't
>>> want/need to publish those streams. Also, sending this message every
>>> time we send a stream is a wrong usage of the command.
>> 
>> You'll need to ensure that we are not freeing the streams too
>> quickly or leaking them in relayd after your change.
> 
> Publishing the stream does not take a new reference on the stream, it
> just adds it in the stream_list so it becomes visible in the viewer
> thread. So it does not change the lifetime of the stream. And if the
> stream is not published when we remove it we don't try to unpublish it.
> 
>>>> The reason for doing this change should also be documented. What
>>>> behavior is unwanted here from a relayd perspective ?
>>> We are sending this message to the relay (and waiting for the
>>> confirmation) before taking the snapshot of each stream. So, in addition
>>> to being wrong and useless, it adds a considerable delay before taking
>>> the snapshot of each stream.
>>>
>>> Do you agree ?
>> 
>> It looks like a good idea to remove it. I just want us to make sure the
>> snapshot mode does not somehow expect the streams to be published
>> within other parts of the relayd code.
> 
> It looks good.

Allright then, thanks!

Mathieu

> 
> Thanks,
> 
> Julien

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com