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: handle long command output #5821

Merged
merged 3 commits into from
Nov 23, 2023

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Nov 15, 2023

  • Log the commands run by log_vc_info (debug level).
  • Handle the risk of large command output clogging up the buffer causing commands to hang.

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@oliver-sanders oliver-sanders added this to the cylc-8.3.0 milestone Nov 15, 2023
@oliver-sanders oliver-sanders self-assigned this Nov 15, 2023
@oliver-sanders oliver-sanders marked this pull request as ready for review November 15, 2023 16:42
@oliver-sanders oliver-sanders changed the base branch from master to 8.2.x November 15, 2023 16:42
@oliver-sanders oliver-sanders modified the milestones: cylc-8.3.0, cylc-8.2.4 Nov 15, 2023
cylc/flow/pipe_poller.py Outdated Show resolved Hide resolved
Comment on lines +29 to +38
def pipe_poller(proc, *files, chunk_size=4096):
"""Read from a process without hitting buffer issues.

Standin for subprocess.Popen.communicate.

When PIPE'ing from subprocesses, the output goes into a buffer. If the
buffer gets full, the subprocess will hang trying to write to it.

This function polls the process, reading output from the buffers into
memory to prevent them from filling up.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what cases might you call this function that isn't the case of pipe_poller(proc, proc.stdout, proc.stderr)? I.e. when would files be something other than the output of proc?

Copy link
Member Author

@oliver-sanders oliver-sanders Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might only be piping one of stdout/err, so you might want:

pipe_poller(proc, proc.stdout)

Or:

pipe_poller(proc, proc.stderr)

You might also be piping one function into another, redirecting stdout/err into some other kind of buffer or fancy polling other other files/buffers associated with the process?

MetRonnie

This comment was marked as resolved.

@MetRonnie MetRonnie self-requested a review November 15, 2023 17:26
oliver-sanders and others added 2 commits November 16, 2023 10:20
* Log the commands run by log_vc_info (debug level).
* Handle the risk of large command output clogging up the buffer causing
  commands to hang.

..
@oliver-sanders oliver-sanders added the bug Something is wrong :( label Nov 17, 2023
cylc/flow/pipe_poller.py Outdated Show resolved Hide resolved
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems generally sane.

I wonder if it might be good to have a functional test which uses bash timeout to demonstrate that this approach is safer (i.e. to get an actual script to hang with proc.communicate).

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Nov 20, 2023

I don't think using a Bash timeout will replicate the problem being fixed here?

You need to get a process to write so much to stdout/err that it fills Python's buffers.

@oliver-sanders oliver-sanders requested a review from wxtim November 20, 2023 15:44
@wxtim
Copy link
Member

wxtim commented Nov 23, 2023

Have approved - the MacOs Tests look to be failing in a standard way. I've kicked flaky and the tutorial tests just to be sure.

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Nov 23, 2023

The tutorials tests are failing here as on upstream/8.2.x at the install step.

@MetRonnie MetRonnie merged commit 8b2feae into cylc:8.2.x Nov 23, 2023
19 of 23 checks passed
@oliver-sanders oliver-sanders deleted the pipe_poller branch November 23, 2023 12:41
wxtim added a commit to wxtim/cylc that referenced this pull request Dec 5, 2023
…at_runtime

* upstream/master:
  log_vc_info: handle long command output (cylc#5821)
  GH Actions: limit tutorial workflow to Py3.11
  remove cylc task dependencies env var (cylc#5836)
  Refactor.lint (cylc#5718)
  protobuf 4.24.4 upgrade (cylc#5828)
  made reinstall work on multiple workflows
  Fix `IndepQueueManager` test (cylc#5832)
  Lint: Add a check for indentation being 4N spaces. (cylc#5772)
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 this pull request may close these issues.

3 participants