reenable emulate_tty by default

[wjwwood]

This is a follow up of #277

[wjwwood]

This this check, I get tracebacks like this:

[DEBUG] [launch]: Traceback (most recent call last):
  File "/home/william/ros2_ws/build/launch/launch/launch_service.py", line 350, in run
    self.__loop_from_run_thread.run_until_complete(run_loop_task)
  File "/usr/lib/python3.6/asyncio/base_events.py", line 484, in run_until_complete
    return future.result()
  File "/home/william/ros2_ws/build/launch/launch/launch_service.py", line 240, in __run_loop
    await process_one_event_task
  File "/usr/lib/python3.6/asyncio/coroutines.py", line 126, in send
    return self.gen.send(value)
  File "/home/william/ros2_ws/build/launch/launch/launch_service.py", line 177, in _process_one_event
    await self.__process_event(next_event)
  File "/usr/lib/python3.6/asyncio/coroutines.py", line 110, in __next__
    return self.gen.send(None)
  File "/home/william/ros2_ws/build/launch/launch/launch_service.py", line 186, in __process_event
    entities = event_handler.handle(event, self.__context)
  File "/home/william/ros2_ws/build/launch/launch/event_handlers/on_process_io.py", line 73, in handle
    return self.__on_stdout(event)
  File "/home/william/ros2_ws/build/launch/launch/actions/execute_process.py", line 396, in __on_process_stdout
    raise RuntimeError('OnProcessIO (stdout) received after stdout buffer closed.')
RuntimeError: OnProcessIO (stdout) received after stdout buffer closed.

[ERROR] [launch]: Caught exception in launch (see debug for traceback): OnProcessIO (stdout) received after stdout buffer closed.

I believe this because this line buffering logic was not designed in a way to allow for ProcessIO events to occur after the ProcessExit event comes. @ivanpauno do you have any ideas about how we could change it to allow for that race condition?

[ivanpauno]

I'm flushing the buffers after a ProcessExit event.

launch/launch/launch/actions/execute_process.py

Lines 675 to 678 in f82c364

	OnProcessExit(
	target_action=self,
	on_exit=self.__flush_buffers,
	),

.
The method for flushing buffers leaves them in a wrong state for handling another ProcessIO event:

launch/launch/launch/actions/execute_process.py

Lines 434 to 446 in f82c364

	def __flush_buffers(self, event, context):
	with self.__stdout_buffer as buf:
	line = buf.getvalue()
	if line != '':
	self.__stdout_logger.info(
	self.__output_format.format(line=line, this=self)
	)
	with self.__stderr_buffer as buf:
	line = buf.getvalue()
	if line != '':
	self.__stderr_logger.info(
	self.__output_format.format(line=line, this=self)
	)

One easy change, is to restore the buffers to a correct state after handling ProcessExit event.
It will, at least, don't raise an error with a new ProcessIO event.
The only bad thing: I think it's impossible to ensure the buffer will be completely flushed after the process exited.

Another option: Ignore ProcessIO events after flushing the buffers, instead of raising an error.

[ivanpauno]

Raising an error here, as you proposed is also fine. If the daemon node of launch_testing is causing this race condition, I think the best solution is stop using a daemon node.

[hidmic]

Ignore ProcessIO events after flushing the buffers, instead of raising an error.

Hmm, if we cannot guarantee order, neither of those options will do. I don't know how many or how often we get this sort of out-of-band ProcessIO events, but we could stop buffering after ProcessExit.

[ivanpauno]

Hmm, if we cannot guarantee order, neither of those options will do. I don't know how many or how often we get this sort of out-of-band ProcessIO events, but we could stop buffering after ProcessExit.

Sounds reasonable.

[wjwwood]

If the daemon node of launch_testing is causing this race condition, I think the best solution is stop using a daemon node.

That's not the cause, the cause is using a pty for stdout and stderr of the subprocess, which means there is different timing (apparently) for when data becomes available on those pipes and the process exiting. Perhaps instead we should not flush until the stdout and stderr are closed (I think you can setup a protocol event for when that happens).

[wjwwood]

Specifically this one: https://docs.python.org/3/library/asyncio-protocol.html#asyncio.SubprocessProtocol.pipe_connection_lost

I'll need to think about the lifetime of these objects and make sure things stick around long enough.

[hidmic]

@wjwwood does this still apply? Or should we drop it?

[tonynajjar]

Hello, the PR has been opened for 5 years, could we know if you still want to go through with it? If not can the PR be closed?
I'm asking because that's a "wide reach" feature, in the sense that so many ROS users have this flag enabled for all their nodes; it would be nice to get a conclusion on whether this will become the default or not. Thank you