reactphp / filesystem

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

ext-eio support #90

Closed enumag closed 3 years ago

enumag commented 3 years ago

I can see the support was dropped in #79. But there is only a claim that the extension is "unreliable" without any specific problem.

What's the reason? The author of ext-eio told me that you never reported any bug there.

Certainly it's better and faster to use such extension than spawning subprocesses for each filesystem operation, right?

WyriHaximus commented 3 years ago

I can see the support was dropped in #79. But there is only a claim that the extension is "unreliable" without any specific problem.

Because our in implementation ext-eio was unreliable.

What's the reason? The author of ext-eio told me that you never reported any bug there.

It's hard to report a bug if you can only report instability within your context, and not outside it.

Certainly it's better and faster to use such extension than spawning subprocesses for each filesystem operation, right?

Correct, and I've had a long talk with @clue about this and part of the slow progress on this package is the number of issues it has. When I initially started creating this package I want to far with the features it has and I wanted in. As a result, the package is rather big and complex. To counter that I'm going to do a rewrite soon only including reading files, writing files, listing directory contents. Main reason for that is to keep the package simple and hopefully a lot stabler than it is now. Now we often have to kick the build a dozen or two times before it's green, and that's the instability you mentioned. That does mean ext-eio has a chance again but also hoping to add ext-uv and possibly ext-parallel. All those 3 are faster than using child processes to do the work.

enumag commented 3 years ago

I see so "unreliable" wasn't the extension itself but rather your usage and you didn't have the time and energy to maintain it properly. Okay that's understandable. Some simplifications to the scope of the package should indeed help it.

As for ext-uv, the author seems very busy and even the segfault I reported many months ago still remains unfixed (with no reaction at all).

ext-parallel should be fine and a nice addition indeed.

Another thing that might possibly help you is that some time ago I made significant refactoring for amphp/file in https://github.com/amphp/file/pull/38. Before it there were many behavior inconsistencies between the drivers (mainly returning null vs throwing on failure). I fixed all of that with new tests. All tests are adapter-independent and are ran for every adapter to ensure consistency. Several functions which were previously implemented for each adapter separately are now simplified to be implemented outside of the adapters (typically isfile, isdir, mtime and anything that internally calls the stat function). Overall I think it's pretty much the same thing you want to do here.

The thing is that amphp already does support all three extensions - uv, eio and parallel. I recommend you to take an inspiration there when you get to it.

WyriHaximus commented 3 years ago

That is pretty much the plan in a nutshell. Not intending to do any adapter specific test, but have a test suite that will test all of them and enforce equal behaviour across all of them.

As for ext-uv, the author seems very busy and even the segfault I reported many months ago still remains unfixed (with no reaction at all).

Which segfault is this?

enumag commented 3 years ago

Which segfault is this?

https://github.com/bwoebi/php-uv/issues/84

WyriHaximus commented 3 years ago

Which segfault is this?

bwoebi/php-uv#84

Thanks!

Will post here again once the rewrite effort it far enough to also include ext-eio.

WyriHaximus commented 3 years ago

Right so th is afternoon I got a PoC working that can list directories, and read file contents in both ext-eio, and ext-uv. Going to write the functional test for those two next. If you want to follow the progress, check this branch: https://github.com/WyriHaximus-labs/filesystem/tree/rewrite

clue commented 3 years ago

@enumag Thanks for bringing this up, I agree my previous message could have come over as condescending, which I didn't intend. I think ext-eio is an excellent piece of software and there's no evidence it is in any way unstable itself.

As pointed out already, we've dropped this with #79 because our integration turned out to be somewhat unreliable. As @WyriHaximus said, we're currently in the process of restructuring the whole project to provide a more stable solution (#46 and others). I'm looking forward to add back ext-eio in the future once we've sorted this out :+1:

I believe this has been answered, so I'm closing this for now. Please come back with more details if you feel this needs more input and we can always reopen this :+1: