filesystem icon indicating copy to clipboard operation
filesystem copied to clipboard

Delay when using Filesystem compared to ReadableResourceStream directly

Open jeromegamez opened this issue 4 years ago • 3 comments

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:

jeromegamez avatar Jul 20 '20 09:07 jeromegamez

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 :)

WyriHaximus avatar Aug 09 '20 21:08 WyriHaximus

@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:

clue avatar Aug 12 '20 16:08 clue

@clue makes sense 👍 . Will have a look at this soon

WyriHaximus avatar Aug 16 '20 14:08 WyriHaximus