Recover a stream with the same uid

[tstenner]

Minimal non-working example:

lsl::stream_outlet outlet(lsl::stream_info("resolvetest", "from_streaminfo"));
	lsl::stream_inlet in(outlet.info());
in.open_stream(2);

At this point, the stream info has the service ports set, but not the IP address (after all, the host could have multiple IP addresses) so the recovery kicks in and searches for a stream named "resolvetest".

This fails, because the stream's uid is known so the recovery process is aborted as soon as the stream is found:

liblsl/src/inlet_connection.cpp

Lines 220 to 222 in 999796e

	for (std::size_t k=0;k<infos.size();k++)
	if (infos[k].uid() == host_info_.uid())
	return; // in this case there is no need to recover (we're still fine)

Possible solutions:

• unset the UID when constructing an inlet from a stream info
• (re)connect to the stream anyways, not a good idea in case the between-sample interval is bigger than the watchdog interval
• add a force_reconnect parameter to try_recover

[tstenner]

CC @chkothe as he knows most about it.

[mgrivich]

That's not the way the examples work. At most, the code should throw an error to say, "don't do that". The inlet info should be gathered from a resolve command, not by reusing the outlet info.

…

On 9/10/2019 8:12 AM, Tristan Stenner wrote: Minimal non-working example: |lsl::stream_outlet outlet(lsl::stream_info("resolvetest", "from_streaminfo")); lsl::stream_inlet in(outlet.info()); in.open_stream(2); | At this point, the stream info has the service ports set, but not the IP address (after all, the host could have multiple IP addresses) so the recovery kicks in and searches for a stream named "resolvetest". This fails, because the stream's uid is known so the recovery process is aborted as soon as the stream is found: https://github.com/sccn/liblsl/blob/999796eb996dca49e7fe283ebfd2a14d32b053a6/src/inlet_connection.cpp#L220-L222 Possible solutions: * unset the UID when constructing an inlet from a stream info * (re)connect to the stream anyways, not a good idea in case the between-sample interval is bigger than the watchdog interval * add a |force_reconnect| parameter to |try_recover| — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#35?email_source=notifications&email_token=ABILPXMMPGBKI3CFU3T3V6LQI62O5A5CNFSM4IVJBJTKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HKPJ2MA>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABILPXPAFZLE3QCPG565I3TQI62O5ANCNFSM4IVJBJTA>.

[chkothe]

I agree that the inlets in their current form aren't meant to be initialized with anything other than the result of a resolve operation. But indeed some users may feel tempted to try that, so having an error that says please use the output of a resolve operation here would fix that. At the risk of sounding a bit defensive, I'd be a little uncomfortable retrofitting a patch into a core piece of the lib that we're basically sure works correctly, just so that a user has their way who's probably better off using a queue data structure instead of LSL anyway, since they're obviously moving data from one end of their own process to another. While also keeping in mind that the resolve would be a way for them to accomplish their goal. If we had a giant test battery that tests all possible ways in which these pieces could malfunction if a change of that sort overlooked a non-obvious detail, then we could probably add coverage for a lot more niche cases.

[tstenner]

That's not the way the examples work. At most, the code should throw an
error to say, "don't do that". The inlet info should be gathered from a
resolve command, not by reusing the outlet info.

In theory yes, but the inlet construction code currently goes out of its way to allow this explicitely:

liblsl/src/inlet_connection.cpp

Lines 58 to 65 in 999796e

	// the actual endpoint is not known yet -- we need to discover it later on the fly
	// check that all the necessary information for this has been fully specified
	if (type_info_.name().empty() && type_info_.type().empty() && type_info_.source_id().empty())
	throw std::invalid_argument("When creating an inlet with a constructed (instead of resolved) stream_info, you must assign at least the name, type or source_id of the desired stream.");
	if (type_info_.channel_count() == 0)
	throw std::invalid_argument("When creating an inlet with a constructed (instead of resolved) stream_info, you must assign a nonzero channel count.");
	if (type_info_.channel_format() == cft_undefined)
	throw std::invalid_argument("When creating an inlet with a constructed (instead of resolved) stream_info, you must assign a channel format.");

I agree that the inlets in their current form aren't meant to be initialized with anything other than the result of a resolve operation.

It would simplify the LabRecorder code a lot because then it could just create an inlet for an outlet that's going to appear some time later.

If we had a giant test battery that tests all possible ways in which these pieces could malfunction if a change of that sort overlooked a non-obvious detail, then we could probably add coverage for a lot more niche cases.

That's where this issue came up. @mgrivich started a battery of unit tests last year and I'm expanding them from time to time. Right now they run after each commit and for each PR to make sure nothing covered breaks.