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

log_vc_info: support large diffs #5117

Closed
oliver-sanders opened this issue Sep 5, 2022 · 3 comments · Fixed by #5139
Closed

log_vc_info: support large diffs #5117

oliver-sanders opened this issue Sep 5, 2022 · 3 comments · Fixed by #5139
Assignees
Labels
bug Something is wrong :(
Milestone

Comments

@oliver-sanders
Copy link
Member

oliver-sanders commented Sep 5, 2022

I've seen a case where svn diff --internal-diff --non-interactive was hanging for some reason. Not sure why just yet but it might be worth putting a default timeout on svn commands called by the log_vc_info plugin to be safe.

Tagged against 8.0.2 but not a critical priority unless others run into it.

When the process was killed the only logged error was:

PluginError: Error in plugin cylc.post_install.log_vc_info
OSError: [Errno -9]

The -9 presumably coming from kill -9 looking at the code.

Pull requests welcome!
This is an Open Source project - please consider contributing a bug fix
yourself (please read CONTRIBUTING.md before starting any work though).

@oliver-sanders oliver-sanders added bug Something is wrong :( question Flag this as a question for the next Cylc project meeting. labels Sep 5, 2022
@oliver-sanders oliver-sanders added this to the 8.0.2 milestone Sep 5, 2022
@oliver-sanders
Copy link
Member Author

The project diff was ~62k lines. Because of the size of the diff the svn diff had exhausted Python's buffer causing the svn command to hang (write blocked).

There is some special logic in the subprocpool which handles this case. Either we port this logic into the subprocess calls that log_vc_info makes or we get log_vc_info to use the subprocpool to make its calls.

@oliver-sanders oliver-sanders removed the question Flag this as a question for the next Cylc project meeting. label Sep 5, 2022
@oliver-sanders
Copy link
Member Author

(Removing the question label, this was about timeouts)

@oliver-sanders
Copy link
Member Author

def _poll_proc_pipes(self, proc, ctx):
"""Poll STDOUT/ERR of proc and read some data if possible.
This helps to unblock the command by unblocking its pipes.
"""
if self.pipepoller is None:
return # select.poll not supported on this OS
for handle in [proc.stdout, proc.stderr]:
if not handle.closed:
self.pipepoller.register(handle.fileno(), self.POLLREAD)
while True:
fileno_list = [
fileno
for fileno, event in self.pipepoller.poll(0.0)
if event & self.POLLREAD]
if not fileno_list:
# Nothing readable
break
received_data = []
for fileno in fileno_list:
# If a file handle is readable, read something from it, add
# results into the command context object's `.out` or `.err`,
# whichever is relevant. To avoid blocking:
# 1. Use `os.read` here instead of `file.read` to avoid any
# buffering that may cause the file handle to block.
# 2. Call os.read only once after a poll. Poll again before
# another read - otherwise the os.read call may block.
try:
data = os.read(fileno, 65536).decode() # 64K
except OSError:
continue
received_data.append(data != '')
if fileno == proc.stdout.fileno():
if ctx.out is None:
ctx.out = ''
ctx.out += data
elif fileno == proc.stderr.fileno():
if ctx.err is None:
ctx.err = ''
ctx.err += data
if received_data and not all(received_data):
# if no data was pushed down the pipe exit the polling loop,
# we can always re-enter the polling loop later if there is
# more data
# NOTE: this suppresses an infinite polling-loop observed
# on darwin see:
# https://github.com/cylc/cylc-flow/issues/3535
# https://github.com/cylc/cylc-flow/pull/3543
return
self.pipepoller.unregister(proc.stdout.fileno())
self.pipepoller.unregister(proc.stderr.fileno())

@oliver-sanders oliver-sanders changed the title log_vc_info: add timeout to commands log_vc_info: support large diffs Sep 12, 2022
@oliver-sanders oliver-sanders modified the milestones: cylc-8.0.2, cylc-8.0.3 Sep 12, 2022
@MetRonnie MetRonnie self-assigned this Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants