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

handle_external_tool_process returns out/err as bytes if empty, as string if not empty #670

Open
ptbannister opened this issue Apr 10, 2018 · 0 comments

Comments

@ptbannister
Copy link
Contributor

Short summary: on the cassandra-test branch, many ccmlib/node.py nodetool functions (anything that goes through handle_external_tool_process) are inconsistently returning stdout and stderr as strings if non-empty, but as bytes if empty. I'd like to update handle_external_tool_process to return strings consistently for stdout and stderr.

Details...

On branch cassandra-test, in ccmlib/node.py handle_external_tool_process returns stdout and stderr as string objects if they're non-empty, but as bytes objects if they're empty. This sets up users for one error or another unless they're careful about checking whether they got a string or a bytes before using the returned values, and it would be preferable to consistently return one type or another.

Details:

def handle_external_tool_process(process, cmd_args):
    out, err = process.communicate()
    if out and isinstance(out, bytes):
        out = out.decode()
    if err and isinstance(err, bytes):
        err = err.decode()
    rc = process.returncode

    if rc != 0:
        raise ToolError(cmd_args, rc, out, err) 

    ret = namedtuple('Subprocess_Return', 'stdout stderr rc')
    return ret(stdout=out, stderr=err, rc=rc)

The inconsistency arises from the checks "if out" and "if err". An empty bytes object (b'') evaluates to False, so if one of these streams is empty, it doesn't get decoded, and it gets returned as an empty bytes object.

I'd like to change the conditional tests "if out" and "if err" to "if (out is not None)" and "if (err is not None)", as below:

def handle_external_tool_process(process, cmd_args):
    out, err = process.communicate()
    if (out is not None) and isinstance(out, bytes):
        out = out.decode()
    if (err is not None) and isinstance(err, bytes):
        err = err.decode()
    rc = process.returncode

    if rc != 0:
        raise ToolError(cmd_args, rc, out, err) 

    ret = namedtuple('Subprocess_Return', 'stdout stderr rc')
    return ret(stdout=out, stderr=err, rc=rc)

I'll submit a PR for the change.

ptnapoleon added a commit that referenced this issue Apr 12, 2018
Issue #670: also decode empty stdout and stderr to strings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant