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

support large arguments #3

Merged
merged 6 commits into from
Feb 4, 2018
Merged

Conversation

RSulzmann
Copy link
Contributor

There is a limit of the argument size in a command line (MAX_ARG_STRLEN = 131071 bytes) .
To support larger sizes, the input to the action code is passed via standard input.

To support also legacy action code (not reading input from stdin), but only if the input size is smaller than the limit, the input is also passed as a command line parameter.

@rabbah
Copy link
Member

rabbah commented Jan 24, 2018

Closes apache/openwhisk#2542.

@jthomas can you review.

stderr=subprocess.PIPE,
env=env)
else:
# pass argument via stdin and command parameter
Copy link
Member

Choose a reason for hiding this comment

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

I suggest that we always do this. That is we migrate all actions to use the stdin path instead of argv[1]. The code above will be a fail safe for all existing actions which will only work up to 128K.

Copy link
Member

@jthomas jthomas Jan 26, 2018

Choose a reason for hiding this comment

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

I agree with this. stdin should be the default. Input can be passed as an argument as well but not if it's larger than 128K.

@rabbah
Copy link
Member

rabbah commented Jan 24, 2018

We will need to add a test. You can use this example, which I did for swift to test the change.
https://github.com/apache/incubator-openwhisk/compare/master...rabbah:arglist?expand=1#diff-cd4d7cf77b5a623c67fc14b0de34cc1aR165

@jthomas
Copy link
Member

jthomas commented Jan 26, 2018

LGTM.

rabbah
rabbah previously requested changes Jan 26, 2018
Copy link
Member

@rabbah rabbah left a comment

Choose a reason for hiding this comment

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

See comments.

jthomas added a commit to jthomas/incubator-openwhisk-runtime-swift that referenced this pull request Feb 2, 2018
Copy link
Member

@csantanapr csantanapr left a comment

Choose a reason for hiding this comment

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

like @rabbah already said it needs a test

Copy link
Member

@rabbah rabbah left a comment

Choose a reason for hiding this comment

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

We could have made an addition to the standard tests all the runtimes run (bash, Perl, Swift, python) to check no regressions in the future for the large inputs.

stderr=subprocess.PIPE,
env=env)
if len(input) > 131071: # MAX_ARG_STRLEN (131071) linux/binfmts.h
# pass argument via stdin
Copy link
Member

Choose a reason for hiding this comment

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

The if/else could be simpler.

Copy link
Member

Choose a reason for hiding this comment

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

I agree and can also read your mind 😉
Send the PR and I will merge

@csantanapr
Copy link
Member

We could have made an addition to the standard tests all the runtimes run (bash, Perl, Swift, python) to check no regressions in the future for the large inputs.

Swift? There is no swift in this repo
Though about it but didn’t think there was a need since we are testing the proxy and not the programming language Perl or python, but easy to add you want to do the PR?

@csantanapr
Copy link
Member

I think I will add the tests to the std ones don’t worry

@rabbah
Copy link
Member

rabbah commented Feb 4, 2018

👍 Thanks. I was basically thinking adding a variant of the echo test would do (thestdCodeSamples) with reading/writing from stdin instead of argv[1], and then making the test you added run against that with the 1MB payload part of testEcho(stdCodeSamples).

@csantanapr
Copy link
Member

Yep you read my mind 💯

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

Successfully merging this pull request may close these issues.

None yet

4 participants