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

Rework file stream API #46

Open
WyriHaximus opened this issue Sep 22, 2018 · 4 comments
Open

Rework file stream API #46

WyriHaximus opened this issue Sep 22, 2018 · 4 comments
Milestone

Comments

@WyriHaximus
Copy link
Member

WyriHaximus commented Sep 22, 2018

Rework file stream API if it is a bit wonky.

@WyriHaximus WyriHaximus added this to the v0.2.0 milestone Sep 22, 2018
@ghost ghost mentioned this issue Sep 22, 2018
@clue
Copy link
Member

clue commented Oct 28, 2018

What are your plans for the API, do you have anything in mind?

I agree that the API surface is much more complex than everything I've ever needed for my (perhaps somewhat simple) use cases. I would welcome to have something like read(string $path): PromiseInterface<string> and readStream(string $path): ReadableStreamInterface<string> more prominently instead of exposing low-level file and directory objects.

@WyriHaximus
Copy link
Member Author

I should have work this issue out better as this is really about streaming the contents of a directory. @CharlotteDunois is already working on improving the reading and writing to and from files.

To simplify that API I'd like to collapse both into read(string $path): Observable using RxPHP. To clarify the read() method is for use cases where you only get a small set of results where readStream() is to be used when the call will yield a lot of results and will cause increased memory usage. Now when using observables we can easily do both.

For small loads:

$directoryContents = Promise::fromObservable($filesystem->read(__DIR__)->toArray());

For large loads:

$filesystem->read(__DIR__)->subscribe(function (NodeInterface $node) {
   // Do something with $node
});

Using observables we can filter out files, directories, or symlinks:

$filesystem->read(__DIR__)->filter(function (NodeInterface $node) {
    // Filter out everything by files
    return $node instanceof FileInterface;
})->subscribe(function (NodeInterface $node) {
   // Do something with $node
});

Or to go even more advanced filter our all files above 1KB:

$filesystem->read(__DIR__)->filter(function (NodeInterface $node) {
    return $node instanceof FileInterface;
})->filter(function (FileInterface $file) {
    // This example is actually more complicated because we have to deal with a promise but this is the basic idea
	return $file->size() < 1024;
})->subscribe(function (FileInterface $file) {
   // Do something with $file
});

@ghost
Copy link

ghost commented Oct 29, 2018

For the File API I want to add a new method (for writing also one), which is an optimization for the adapters child process and pthreads to the current behaviour/method. This new method will collapse the current calls open -> read/write (recursive) -> close into one method call, which is an useful optimization for small files.

Currently the File nodes invokes three methods (one maybe more than once), which leads to minimum three calls to the adapter implementations. The mentioned two adapters can use file_get_contents (respectively file_put_contents), which makes the engine handle opening and closing automatically - and it removes minimum two adapter calls.

This would be used for File::getContents (and File::putContents respectively for writing). For Eio we can not do this optimization on the adapter level, but instead the adapter will invoke the three methods.

Something along this (for Eio):

function readFile($path) {
    return $this->open($path)->then(function ($fd) {
        return $this->read($fd, 0, PHP_INT_MAX)->always(function () use ($fd) {
            $this->close($fd);
        });
    });
}

@WyriHaximus This would remove a method to check whether the adapter supports this - I think this makes more sense to do (the adapter handling this instead).

@WyriHaximus
Copy link
Member Author

@CharlotteDunois sorry for the late response, but go for it, this makes a lot of sense 👍

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

Successfully merging a pull request may close this issue.

2 participants