reactphp / filesystem

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

Reliability issues when writing multiple files at the same time #96

Closed DevMcC closed 3 years ago

DevMcC commented 3 years ago

Hi, we've been working on an export, which should export 5 files (relatively at the same time).

However we've been experiencing some reliability issues, sometimes a file is being fully written, sometimes not and sometimes the export just gets stuck. We're running every test with the same data, but we get different results every time. This issue is especially noticable for OSX, but we're now experiencing the same issue with Linux.

The way we have written it in the code is like this:

public function export(FilesystemInterface $filesystem, File $file): PromiseInterface
{
    return new Promise(function (callable $resolve) use ($filesystem, $file) {
        $fullPath = $this->fullPath($file);
        $filesystem->file($fullPath)->open('cwt')->then(function ($stream) use ($resolve, $file) {
            // This part is simplified, just to show an example.
            foreach ($file->lines() as $line) {
                $stream->write($this->format($line));
            }
            $stream->end();
            $resolve();
        });
    });
}

This method gets called 5 times, we collect all the promises and then we start the export with \Clue\React\Block\awaitAll($promises, $loop);.

Is it possible to write multiple files at the same time? Are we doing something wrong here?

Update: We've temporarily modified the export to only export one file, but even this seems to not work. Even on Linux we're experiencing more misfires now, the files are just empty.

kelunik commented 3 years ago

You'll need to subscribe to the close event on the stream before calling $resolve(), as writing might not have finished, yet.

WyriHaximus commented 3 years ago

Hey @DevMcC,

Do you know which adapter is used? E.g. is it the EIO or the child process adapter?

Currently busy doing a full rewrite, and one of the changes is a promise based writing API. So each write will return a promise and you'll have to wait. This also resolves the issue @kelunik is mentioning now.

DevMcC commented 3 years ago

@kelunik, I've changed the code into this:

public function export(FilesystemInterface $filesystem, File $file): PromiseInterface
{
    return new Promise(function (callable $resolve) use ($filesystem, $file) {
        $fullPath = $this->fullPath($file);
        $filesystem->file($fullPath)->open('cwt')->then(function ($stream) use ($resolve, $file) {
            $stream->on('close', function () use ($resolve) {
                $resolve();
            });

            // This part is simplified, just to show an example.
            foreach ($file->lines() as $line) {
                $stream->write($this->format($line));
            }
            $stream->end();
        });
    });
}

This has certainly improved the reliability. However the export is sometimes still getting stuck, the then from ->open('cwt') isn't always called.

@WyriHaximus, we're using the ChildProcess adapter.

WyriHaximus commented 3 years ago

This has certainly improved the reliability. However the export is sometimes still getting stuck, the then from ->open('cwt') isn't always called.

Which means it gets stuck somewhere along the line. Opening the file hmm

@WyriHaximus, we're using the ChildProcess adapter.

The ChildProcess adapter is mainly there as a fallback to ensure it works everywhere. You could try install ext-eio for better performance. Added additional tests to the rewrite to ensure writing to multiple files isn't an issue. Got it up to 2000+ for the ext-uv adapter in there.

DevMcC commented 3 years ago

Which means it gets stuck somewhere along the line. Opening the file hmm

I did some debugging and it turns out that the file export is a bit slow, some files have a lot of data to export. This slowness seems to cause a TimeoutException for the other files that haven't been run yet.

I don't think going around this TimeoutException would be a good idea, I think it would be better to restructure the export. Instead of winging 5 promises, we could process the files synchronously and then store the content asynchronously. I'll give an update on how that goes.

The ChildProcess adapter is mainly there as a fallback to ensure it works everywhere. You could try install ext-eio for better performance.

Sure, I'll try it.

Added additional tests to the rewrite to ensure writing to multiple files isn't an issue. Got it up to 2000+ for the ext-uv adapter in there.

Cool, I'm looking forward to the rewrite.

DevMcC commented 3 years ago

Hi, I've got an update.

We didn't go for ext-eio, as according to the GitHub issues, this repo's implementation of that extension is unreliable.

I don't think going around this TimeoutException would be a good idea, I think it would be better to restructure the export. Instead of winging 5 promises, we could process the files synchronously and then store the content asynchronously. I'll give an update on how that goes.

We ended up going with something like this. Instead of awaitAll, we use await for a single file and then move on to the next file. The foreach was changed to a periodicTimer, so the file is being written as we're receiving the lines.

This approach made the export very stable, which is really nice.

I consider the reliability issue resolved, I'll be closing this now.