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

Infinite looping timeout #325

Closed
Zyles opened this issue Jan 24, 2019 · 14 comments
Closed

Infinite looping timeout #325

Zyles opened this issue Jan 24, 2019 · 14 comments
Assignees
Labels

Comments

@Zyles
Copy link

Zyles commented Jan 24, 2019

Bug Report

Information Description
Version ^9.1
PHP version PHP 7.3.1-1+ubuntu14.04.1+deb.sury.org+1
OS Platform ubuntu 14.04

Summary

echo $csv->getContent(); causes an infinite loop and eventually nginx gateway timeout.

$header = $csv->getHeader();
print_r($header);

Array
(
    [0] => Hello;Foobar
)

The CSV file consists of:

Hello;Foobar
Hi;There

Expected result

Stop timing out.

Actual result

Timeout.

@nyamsprod
Copy link
Member

@Zyles thanks for the bug report but I fail to reproduce your issue using your code. Did you manage to solve it in the meantime ? because looking at your code I don't get why nginx would be affected by the library... I need more detail to investigate

@Zyles
Copy link
Author

Zyles commented Jan 28, 2019

Well I don't know, it just keeps timing out. Not sure how to debug it any further.

I went with the statement solution instead which works for me.

@nyamsprod
Copy link
Member

@Zyles is it possible to get the command used only those related to League\Csv so I get try to reproduce the bug. The bug is about getContent and you seems to have resolve it using Statement but internally both method uses the same code so I have to wonder 🤔 .

  • Do you use a separate file or the CSV is produced from a stream ❓
  • Is it build from or on a Mac OS ❓
  • Does you nginx supports PHP buffering ❓

Having acces to the script would help in knowing if it is a bug or a misconfiguration of the library.

@nyamsprod
Copy link
Member

closing since no feedback was provided

@kpion
Copy link

kpion commented Jun 3, 2019

@nyamsprod - I just happened to see a very similar issue - maybe I misunderstand something but the following lines just don't work:

// it does not matter if we load it from a file or from string
// $csv = Reader::createFromPath('./input/test.csv', 'r');
$csv = Reader::createFromString("col1,col2\nv1a,v2a\nv1b,v2b");

// but it does matter if we call the getRecords. Because if we do, the 
// ::getContent will never end, it will go into an infinite loop.

$records = $csv->getRecords(); 
foreach($records as $r){
     print_r($r);
}
echo $csv->getContent(); //returns the CSV document as a string    

I'm sure I'm missing something? But can't see what that would be o.O I think that even if I missued it, it shouldn't behave like this :)

Once again - everything works fine if you'll remove the $csv->getRecords(); and then the foreach.

On apache, PHP 7.2.15-1+ubuntu16.04.1+deb.sury.org+1 (cli) (built: Feb 8 2019 15:37:29) ( NTS )

Oh, here is an edit:

Do you use a separate file or the CSV is produced from a stream

nope, a regular, very normal file, but it also fails with a string

Is it build from or on a Mac OS

nope

Does you nginx supports PHP buffering

it's apache, but I also tried on cli, same result.

@nyamsprod
Copy link
Member

@kpion thanks for the code could you try your snippet with Reader::createFromFileObject and see if the issue is still present or not. Depending on your answer I may have found the issue. I'll re-open the issue once I get your feedback if necessary

@kpion
Copy link

kpion commented Jun 4, 2019

@nyamsprod
Thanks.

When using ::createFromFileObject everything's fine, doesn't matter if I do call the ::getRecords() or not.

$fo = new SplFileObject('./input/tests.csv');
$csv = Reader::createFromFileObject($fo);

//now everything's cool, even when I do this stuff:
$records = $csv->getRecords(); 
foreach($records as $r){
     print_r($r);
}
echo $csv->getContent(); //returns the CSV document as a string   

The csv file is the same as in the above examples. It doesn't matter as well though, because when I load this file with ::createFromPath I do have issues.

@nyamsprod
Copy link
Member

nyamsprod commented Jun 4, 2019

@kpion then I know what is the issue 🎉 but It will take time for me to resolve it 😢

Basically there's an issue in the Stream::valid method which seems to not return what's expected under Nginx. Why I don't care but it needs to be fix.

@nyamsprod nyamsprod reopened this Jun 4, 2019
@nyamsprod nyamsprod added the bug label Jun 4, 2019
@kpion
Copy link

kpion commented Jun 4, 2019

OK, well, thanks. Although - two things - it's cli, and also apache. Didn't actually test it under nginx. The OP did :) And second thing - when it comes to me, I did "solve" by not calling the ::getRecords method anywhere.

I made this comment to just let you know.

Thanks for the stuff, and keep the good work!

@nyamsprod
Copy link
Member

@kpion turns out that with your last comment I was able to quickly resolve the issue 👍 🎉 . If you could test the branch in PR I'll merge it and release the bugfixed version ASAP.

@kpion
Copy link

kpion commented Jun 5, 2019

@nyamsprod

Hmm, yes and no :) Seems like we test it differently, or... IDK, maybe I'm doing something wrong.

But no, it did not work. But I did take a look at your changes, you're playing with the SplFileObject flags - that is - in the AbstractCsv.php in the output method you added

$this->document->setFlags(0);

But in my scenario, this method (output) isn't even called. The AbstractCsv::getContent which I use doesn't use it directly or indirectly.

So I modified the above method, i.e. I added this setFlags to it and yeah, now it works:) Here is the complete method now:

    public function getContent(): string
    {
        $this->document->setFlags(0);// here it is
        $raw = '';
        foreach ($this->chunk(8192) as $chunk) {
            $raw .= $chunk;
        }

        return $raw;
    }

My guess is that it's wrong, for example because the flags should possibly be restored or something, but ... it proves it works :)

@nyamsprod
Copy link
Member

Sorry yeah wrong patched method it should have been in the getContent like you did... I'll fix this asap. Good catch

nyamsprod added a commit that referenced this issue Jun 7, 2019
…eout

Bugfix/infinite looping timeout issue #325
@nyamsprod
Copy link
Member

fix merge into master

@kpion
Copy link

kpion commented Jun 7, 2019

Yeah, tested, and it works. Thank you!

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

No branches or pull requests

3 participants