search

reactphp / filesystem

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

Reintroduce the ext-eio adapter #112

Closed WyriHaximus closed 1 year ago

WyriHaximus commented 1 year ago

During the initial rewrite this adapter was removed to make the rewrite easier. Since that is now in adding the ext-eio adapter will provide several high performance adapters.

WyriHaximus commented 1 year ago

@clue updated the PR with findings regarding ext-eio and ext-uv, managed to resolve the issues, details in review thread

lucasnetau commented 1 year ago

Does the new adapter handle when eio_open returns false (or any other call, but eio_open appears to be the most common)?, it's undocumented but if you look into the PECL library you can see the error path that RETURN_FALSE instead of -1

See https://github.com/reactphp/filesystem/issues/43 was the reason we had to move to the child process for file operations

WyriHaximus commented 1 year ago

Does the new adapter handle when eio_open returns false (or any other call, but eio_open appears to be the most common)?, it's undocumented but if you look into the PECL library you can see the error path that RETURN_FALSE instead of -1

As in it returns false, and doesn't call the handler you give it. Or it calls with handler with a false?

See #43 was the reason we had to move to the child process for file operations

Will have a thorough look again at #43, but during development, I haven't run into those issues for 0.2. 0.2 is aimed a keeping things a lot simpler and avoiding making the same mistakes I made in 0.1.

clue commented 1 year ago

@WyriHaximus I've noticed the new eio related classes don't currently have 100% code coverage, is this something you plan to look into? Otherwise no objects to go ahead with this from my end :+1:

WyriHaximus commented 1 year ago

@clue yes, in the next PR I'll address several code coverage issues across the board by covering more situations such as partial reads.

clue commented 1 year ago

Personally, I'd like to see better code coverage for this new adapter to make sure we're not introducing any regressions before moving forward with this. For instance, the stat syscall in getContents() as discussed above is unneeded and most library calls currently have limited error checks – but the same currently applies to all others adapters as well. With this in mind, I don't want to block moving forward with this PR and agree that these are things that can be addressed in follow-up PRs :+1:

@WyriHaximus Let's merge this as is? :shipit:

WyriHaximus commented 1 year ago

@clue Works for me, that is exactly what I'll be addressing in the follow up PR