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

clush: always close stdin stream of worker when it is not used #478

Merged
merged 1 commit into from
Jul 1, 2022

Conversation

thiell
Copy link
Collaborator

@thiell thiell commented Jun 30, 2022

Pass explicit stdin boolean flag when calling Task.shell() to fix an
issue where the stdin stream of the worker is never closed, even if
not used.

This change should not impact any already working stdin workflow.

However, the following command from a tty should not block anymore:

$ clush -w nodes cat

Pass explicit stdin boolean flag when calling Task.shell() to fix an
issue where the stdin stream of the worker is never closed, even if
not used.

This change should not impact any already working stdin workflow.

However, the following command from a tty should not block anymore:

$ clush -w nodes cat
@thiell thiell self-assigned this Jun 30, 2022
@thiell thiell added this to the 1.8.5 milestone Jun 30, 2022
@thiell thiell added the Scripts label Jun 30, 2022
Copy link
Collaborator

@degremont degremont left a comment

Choose a reason for hiding this comment

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

Code wise, looks good, but i'm not sure i can say this is a good or bad idea. I'm approving it, but without lots of confidence (what's pdsh behavior here?)

@thiell
Copy link
Collaborator Author

thiell commented Jun 30, 2022

pdsh doesn't support stdin forwarding/broadcast.
See: https://unix.stackexchange.com/questions/544052/pdsh-input-from-file-possible
BTW, we could add a note re: clush there. :)

@thiell thiell modified the milestones: 1.8.5, 1.9 Jun 30, 2022
Copy link
Collaborator

@martinetd martinetd left a comment

Choose a reason for hiding this comment

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

LGTM.

There's just one change of behavior, that a command expecting things from stdin when stdin is not forwarded quits immediately -- I think that's actually good.

PYTHONPATH=$PWD/lib/ python3 -m ClusterShell.CLI.Clush -w localhost cat

(note even when it wasn't quitting immediately stdin was captured for the interactive side of things, not for cat, so this doesn't break anything)

That aside I couldn't see any difference, and code looks good to me.

Also, I have a deprecation warning on python 3.10

/home/martinet/code/clustershell/lib/ClusterShell/CLI/Clush.py:651: DeprecationWarning: setDaemon() is deprecated, set the daemon attribute instead
  stdin_thread.setDaemon(True)

@thiell
Copy link
Collaborator Author

thiell commented Jul 1, 2022

Also, I have a deprecation warning on python 3.10

Thanks! I missed that as we don't run the tests with Python 3.10 yet, because python-nose is broken. setDaemon() has just been deprecated in 3.10.

The daemon attribute was added in 2.6, so we can definitely use that now. I will submit a separate patch.

There is also a daemon argument for threading.Thread()
Changed in version 3.3: Added the daemon argument. We cannot use that yet as we still support Python < 3.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants