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

3535: prevent subprocpool infinite loop #3543

Merged
merged 3 commits into from
Apr 2, 2020

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Mar 30, 2020

These changes close #3535

This is a potential fix for the infinite loop observed in the subprocpool process polling logic on darwin (BSD).

Not sure what is causing the bug, however, in the Cylc code it surfaces in _poll_proc_pipes where every time we go around the loop we get data == '' even long after the process has completed.

Requirements 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).
  • Appropriate tests are included (impossible to test when you don't know what the underlying issue actually is).
  • No documentation update required.

@oliver-sanders oliver-sanders added the bug Something is wrong :( label Mar 30, 2020
@oliver-sanders oliver-sanders added this to the cylc-8.0a2 milestone Mar 30, 2020
@oliver-sanders oliver-sanders self-assigned this Mar 30, 2020
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Here's what I did to test it:

  • had a look at Travis CI, and it doesn't seem to affect Linux (the code added is not supposed to ever happen from what I understand)
  • downloaded the FreeBSD 12 image
  • installed on VirtualBox
    • resized disks
    • installed Python 3.7, pip, git, and pyzmq
    • installed Cylc Flow from master
    • created cylc-suites/five/suite.rc 😬

Happy to report Cylc Flow works on FreeBSD 12.1. But alas I couldn't reproduce it. Workflow five has been fine since ~4:30PM (it's ~5:30PM now).

Tried PureDarwin but too hard to install. If it fixes the issue on MacOS, then +1 from me, but probably needs @hjoliver and maybe others from MO to review it too.

@kinow
Copy link
Member

kinow commented Mar 31, 2020

Conflict in CHANGES.md @oliver-sanders

And screenshots of Cylc 8 on FreeBSD

image

image

@hjoliver
Copy link
Member

The code looks fine to me, just wondering what it means exactly if the problem is triggered.

@hjoliver
Copy link
Member

Here's what I did to test it: ...

Well done, that's some serious testing 😁

@oliver-sanders
Copy link
Member Author

Tried PureDarwin but too hard to install

Wow @kinow that is dedication to the cause! Darwin is not the easiest OS to handle, I don't know anyone who has actually installed it standalone like that!

When we get this DNS muck sorted out we should use the MacOS platform with GitHub actions to save you the bother of all that!

@oliver-sanders
Copy link
Member Author

Note, 363089e fixes the dependency section of setup.py which seems to have been broken during rebase/merge and will currently cause TB failures on master.

@@ -52,7 +52,7 @@ def find_version(*file_paths):
'protobuf==3.11.*',
'pyzmq==18.1.*',
'click>=7.0',
'psutil>=5.6.0'
'psutil>=5.6.0',
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Tests passed after a kick.

@oliver-sanders
Copy link
Member Author

Merging with two approvals.

@oliver-sanders oliver-sanders merged commit 0a95c00 into cylc:master Apr 2, 2020
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.

subprocpool poller infinite loop bug
3 participants