reactphp / filesystem

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

Idea: ChildProcessAdapter #6

Closed Zizaco closed 5 years ago

Zizaco commented 9 years ago

@WyriHaximus What do you think of creating an adapter (ChildProcessAdapter) that runs a script within the package as a child process using react/child-process to read/write/list files asynchronously?

This way the package would not depend on Eio, also wouldn't it be more stable?

WyriHaximus commented 9 years ago

Love the idea, and would definitely love to see a PR implementing that. My focus for now will be on completing the EIO adapter and the rest of the objects until it's stable. The way I've been setting up the project is that you can swap out EioAdapter with another adapter performing the same operations using a different adapter. #4 was already mentioning that.

EIO is a beast that needs to be carefully controlled. It's very powerful but it needs to be tamed and controlled. So either it's more stable or not to use react/child-process is up for debate. Running this in production already for several projects and it's holding up stable there but not without learning all EIO's little quirks.

Still not there yet, ran into a small design flaw today regarding file permissions that I need to fix.

The ultimate end goal is to have react/filesystem work out of the box without any extensions but also include EIO and LibUV adapters for those looking for more performance :).

Zizaco commented 9 years ago

Great! I'll start working on this new Adapter then.

I saw a blogpost of yours where you commented about using a pool to limit the filesystem calls. Do you have a design in mind? (Where the pool size would be specified? In the Filesystem instance?) I can try to put the call pool into practice in ChildProcessAdapter to avoid running too many processes in parallel. What do you think?

WyriHaximus commented 9 years ago

Awesome!

Currently PooledInvoker does that behavior in a miniature way. But for this kind of pooling I don't have a design in mind. In fact I'm going to design it for a personal OSS package anyway so I'de love to hear your ideas on it :smile: .

Also can you once you committed something to your fork just create a PR so I can track your progress?

joshdifabio commented 9 years ago

I'd definitely like to see an adapter which relies on child processes; the dependency on EIO makes it quite difficult to use this package at present.

I saw a blogpost of yours where you commented about using a pool to limit the filesystem calls. Do you have a design in mind? (Where the pool size would be specified? In the Filesystem instance?) I can try to put the call pool into practice in ChildProcessAdapter to avoid running too many processes in parallel. What do you think?

I've faced the same challenge a number of times recently. Take a look at joshdifabio/resource-pool.

WyriHaximus commented 9 years ago

I'd definitely like to see an adapter which relies on child processes; the dependency on EIO makes it quite difficult to use this package at present.

Why does the EIO dependency make is quite difficult for you? Just interested in the why no matter how obvious. A child process adapter is definitely on my list and tbh I was hoping @Zizaco would pick it up and get it on track but not seeing progress on this end. At the moment I'm focusing on polishing everything and building an S3 adapter wyrihaximus/react-filesystem-s3. (One of the bonuses of working on it is that I find odd bugs before this component gets tagged.) So if either @Zizaco or you want to work on the child process adapter it would be greatly appreciated. Otherwise it will be done eventually.

I've faced the same challenge a number of times recently. Take a look at joshdifabio/resource-pool.

Same here, that's why I've created wyrihaximus/react-child-process-pool which uses child processes to pool the workload

joshdifabio commented 9 years ago

Why does the EIO dependency make is quite difficult for you?

Purely because of the dependency on an extension.

WyriHaximus commented 9 years ago

Purely because of the dependency on an extension.

Fair enough, it's on the list and just realized I also have to work out the adapter interface documentation better so building that becomes easier :). (The goal is to have two extension requiring adapters and one adapter that works everywhere included.)

ghost commented 5 years ago

@WyriHaximus This issue should be closed by now.

clue commented 5 years ago

I agree that this can be closed. Can you attach this to the right milestone and/or share which minimum version is requires for this adapter? :v:

WyriHaximus commented 5 years ago

Done 👍