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

ExecUtil.executeCommandLine() is broken #1677

Closed
ivmarkov opened this issue Sep 30, 2020 · 4 comments · Fixed by #1700
Closed

ExecUtil.executeCommandLine() is broken #1677

ivmarkov opened this issue Sep 30, 2020 · 4 comments · Fixed by #1700
Labels
bug An unexpected problem or unintended behavior of the Core
Milestone

Comments

@ivmarkov
Copy link

I assume that the semantics of e.g. ExecUtil.executeCommandLine("ls .") is to execute the command "ls" with a parameter ".".

(There is some sort of "extra safety" variant where I can pass "ls@@." and it will tokenize it etc. to workaround some "issues" on Mac, as it says. Looks a bit smelly, but anyways.)

The problem is when the caller does NOT use the "@@" trick. Passing "ls ." (i.e. where regular whitespace is used) will not do the right thing anymore, because someone has switched the implementation to ProcessBuilder on master several months ago. ProcessBuilder (unlike Runtime.exec() which was seemingly used in 2.5) does not accept a single command-line string where the command and its arguments are separated by whitespace.

Calling new ProcessBuilder(cmdLine) deceptively seems to work because the class does have a constructor with (String... commands) signature so it takes a single String parameter, but that parameter is supposed to be the command ONLY, not the whole command-line.

At least one binding in 3.0 (SamsungTV) seems to be broken because of this issue.

@ivmarkov
Copy link
Author

By the way, there could be another issue: you are (trying to) exhaust the input stream (= stdout) of the child process after calling waitFor(). If memory serves me right, you have to do it before you are stuck in waitFor, or the child process might block when it fills its buffer and starts waiting on you to read it. And you need to exhaust stderr as well.

Quick googling here seems to confirm this: https://www.xspdf.com/help/50395592.html

So it will work where the child process returns only a handful of characters. Try it with a child which dumps gobs of info on stdout and it will block.

@kaikreuzer
Copy link
Member

Sounds like a regression then - I'll put it to the openHAB 3 project, so that it is tracked.

@kaikreuzer kaikreuzer added the bug An unexpected problem or unintended behavior of the Core label Oct 4, 2020
@wborn wborn added this to the 3.0 milestone Oct 6, 2020
@andrewfg
Copy link
Contributor

By the way, there could be another issue: you are (trying to) exhaust the input stream (= stdout) of the child process after calling waitFor(). If memory serves me right, you have to do it before you are stuck in waitFor, or the child process might block when it fills its buffer and starts waiting on you to read it. And you need to exhaust stderr as well.

@ivmarkov / @kaikreuzer -- I think there is still an issue here.

  • If you run executeCommandLine() on a "fast" command such as ls then it does return the full console output, but
  • If you run it on a "slow" command (such as wget or curl) it does NOT return the full console output; it returns an empty string

https://community.openhab.org/t/oh3-migrated-rule-executecommandline-command-fails/114027

@openhab-bot
Copy link
Collaborator

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/oh3-migrated-rule-executecommandline-command-fails/114027/7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants