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

Fix case where POST body is the single character "0". #133

Merged
merged 4 commits into from May 21, 2019

Conversation

olsonanl
Copy link
Contributor

This patch fixes the case where a POST request is the single character "0" (zero). Stock starman will hang in that case trying to read from a stream which will contain no more data.

@miyagawa
Copy link
Owner

This looks good to me, any idea why the CI tests are failing?

@olsonanl
Copy link
Contributor Author

Hm, no. I'll see if I can get the test environment going here and replicate the problem. I'm actually not sure if the defined test is completely correct; it might be safer to check for '' since there are places in the code where '' is assigned (but they may come after the initial initialization where this problem crops up).

@miyagawa
Copy link
Owner

I pulled your change locally to see if the failure is CI specific and got the same failure.

@miyagawa
Copy link
Owner

I think the issue is that it's only checking defined and inputbuf could be empty ("").

@olsonanl
Copy link
Contributor Author

olsonanl commented May 20, 2019 via email

@miyagawa
Copy link
Owner

It would be nice if you can add a unit test to illustrate the original problem (where content body is "0").

@olsonanl
Copy link
Contributor Author

OK - I'll try to do that as well.

Add single_zero.t for testing this case. Note that in the failure case it hangs;
I don't know the test-fu for properly handling that.
@olsonanl
Copy link
Contributor Author

olsonanl commented May 20, 2019 via email

$ENV{PLACK_SERVER} = 'Starman';

my $app = sub {
my $env = shift;
Copy link
Owner

Choose a reason for hiding this comment

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

you should probably check if $req->content is "0"

@olsonanl
Copy link
Contributor Author

Like that?

@miyagawa miyagawa merged commit 2d258b4 into miyagawa:master May 21, 2019
@miyagawa
Copy link
Owner

thanks!

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

2 participants