reactphp / filesystem

Evented filesystem access.
MIT License
135 stars 40 forks source link

Delay when using Filesystem compared to ReadableResourceStream directly #85

Open jeromegamez opened 4 years ago

jeromegamez commented 4 years ago

Greetings πŸ‘‹,

I'm fairly new to ReactPHP, please forgive me if this is something obvious and my Google/Stack Overflow search skills have just failed me πŸ™ˆ.

"react/filesystem": "^0.1.2" is the only dependency in my composer.json, and I'm using PHP 7.4.8 installed with Homebrew on MacOS Big Sur (I hope that's not the reason πŸ˜…), with the StreamSelectLoop (no event extensions are installed).

The following code hopefully makes it reproducible:

// test.php
use React\EventLoop\Factory;
use React\Filesystem\Filesystem;
use React\Stream\ReadableResourceStream;

$loop = Factory::create();

$filePath = 'test.txt';

$file = new ReadableResourceStream(fopen($filePath, 'r'), $loop);
$fs = Filesystem::create($loop);

When using the ReadableResourceStream, the text is printed and the script terminates immediately:

$file->on('data', function ($contents) {
    echo $contents;
});

$loop->run();
❯ time php test.php
test
php test.php  0,04s user 0,03s system 94% cpu 0,070 total

When using the Filesystem, the text is printed immediately, but it takes another three to four seconds until the script finally terminates:

$fs->getContents($filePath)
    ->then(function ($contents) {
        echo $contents;
    });

$loop->run();
❯ time php test.php
test
php test.php  0,15s user 0,11s system 6% cpu 4,083 total

I'm not sure if this is an actual problem or if I missed something in the docs - either way I'd be happy if you could give me a hint on what might be going on here. (I also noticed that the system time is quite different between the two methods)

:octocat:

WyriHaximus commented 4 years ago

This isn't so much as a bug, but as a consequence of child processes or threads to do the IO. It takes a bit to shut them down, where with you first example you use a slightly blocking method of opening files. ALso this is something that shouldn't be an issue with long running processes :)

clue commented 4 years ago

@jeromegamez Thanks for reporting, I agree that this is definitely unexpected behavior! :+1:

@WyriHaximus I understand the technical background, but do we really need to keep these processes around when no other work is outstanding? IMHO we should default to a very small timeout (1ms) with the option of increasing this for long-running processes. Of course spawning child processes incurs some overhead, but I'd rather solve the 80% use case and then look into additional optimizations for more specific use cases. What do you think? :+1:

WyriHaximus commented 4 years ago

@clue makes sense πŸ‘ . Will have a look at this soon