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 too many characters cached in buffer #798

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

huweiATgithub
Copy link

Issue Description

Recently, I've encountered an issue that the launch test (added with add_ros_test with a launch.py file) hangs indefinitely.
After investigation, I found that it was stuck at the getvalue call of a StringIO buffer:

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

The root cause is that the __on_process_output method stores output in the buffer but does not process it, resulting in a huge buffer to be getvalue on (and thus hangs):

for line in buffer:
if line.endswith(os.linesep):
logger.info(
self.__output_format.format(line=line[:-len(os.linesep)], this=self)
)
else:
last_line = line
break

This happens because one of my noisy program prints to stdout without a line separator, causing the buffer to accumulate.

Proposed Improvement

This PR addresses the issue by ensuring the buffer does not store too many characters, which I think it is always not wrong.

Other Changes

  1. I also change the os.linesep to \n since from the doc of StringIO, the StringIO by default uses \n as newline. Otherwise, writing lines with EOL='\n' in Windows will stop line from being processed.
  2. Remove the break in the else clause; this is unnecessary, it will be the last line in the buffer if it does not end with the line separator.

@fujitatomoya
Copy link

@mjcarroll can you assign yourself to this? (i do not have power...)

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.

3 participants