Giter Club home page Giter Club logo

Comments (10)

WyriHaximus avatar WyriHaximus commented on May 16, 2024

Personally I haven't had any issues with it, but would love more exact details on situations where this causes issues so we can add that to the README note

from child-process.

DaveRandom avatar DaveRandom commented on May 16, 2024

This can be avoided at the C level using the FD_CLOEXEC flag (this came up at phpnw in relation to an old xdebug bug report).

With file handles there's no way to set this from userland that I'm aware of, but with sockets it can done with socket_import_stream()/socket_set_option() - it may require passing the option number directly, I don't know if the relevant constant is defined, I haven't checked.

I could look at exposing this via a stream context parameter if there's any interest in it? Likely would not be available until 7.3 though.

Related: https://bugs.php.net/bug.php?id=67383

from child-process.

kelunik avatar kelunik commented on May 16, 2024

@DaveRandom For PHP that's probably good enough, because it's single threaded, but it requires the socket extension then. For files there's the 'e' flag, but probably nobody uses that.

from child-process.

DaveRandom avatar DaveRandom commented on May 16, 2024

@kelunik I feel like $ctx['fd']['inheritable'] = false is the way to go with this, personally (which affects fds so will include sockets)

from child-process.

kelunik avatar kelunik commented on May 16, 2024

Sounds good to me. Maybe we can even backport that to 7.1 / 7.0. Honestly, I don't see many reasons why it shouldn't be the default, because PHP doesn't expose the raw FDs anywhere to let a child use them.

from child-process.

DaveRandom avatar DaveRandom commented on May 16, 2024

@kelunik I would also be OK with that but it would probably need to be done in 8 because semver. For the time being it can default to current (arguably but not necessarily) broken behaviour.

The nice thing about context options is that setting non-existent ones doesn't break anything, so at least existing code can be modified to set it without worrying about whether it will actually have any effect (i.e. no need for a version check, react/amp can just set it by default).

It is possible to get at raw fds in PHP btw, http://php.net/manual/en/wrappers.php.php#wrappers.php.fd

That's not the only consideration though - the child is not necessarily a PHP process, so it's theoretically possible to do interesting/useful stuff with inheritable fds across exec boundaries.

from child-process.

kelunik avatar kelunik commented on May 16, 2024

Yes, it's possible to access the FD, I know, but if you open a stream, you can't get the FD number to let the child process use that (inherited) FD.

from child-process.

DaveRandom avatar DaveRandom commented on May 16, 2024

@kelunik I will look at exposing that via stream_get_meta_data() while I'm at it. May as well make this into a potentially useful (albeit niche) feature.

from child-process.

clue avatar clue commented on May 16, 2024

@DaveRandom Thanks for chiming in, I agree that adding a context option to set this mode on creation is the way forward here πŸ‘

Whether this should be the future default is debatable (I agree), taking a look at how others handle this (such as https://www.python.org/dev/peps/pep-0446/ and https://bugs.ruby-lang.org/issues/5041) suggests that they figured it's worth introducing this as a fix even in a minor version. Either way, this would probably be best discussed with php-internals instead of this issue, so I'm all for pushing this upstream πŸ‘

Exposing access to FDs is only partly related to this issue, but I agree that exposing it via stream_get_meta_data() looks sane to me πŸ‘

Arguably, this is not really an issue in this repository, but more like how underlying Unix system work. Given the limited number of options currently, I would still suggest documenting this as a known issue in the README of this repository. We should still look into providing better defaults in a future version πŸ‘

from child-process.

WyriHaximus avatar WyriHaximus commented on May 16, 2024

Just confirmed this has been solved with #65 by handling it the following way: WyriHaximus/reactphp-child-process-messenger@9995a03#diff-8157a0d28a58887c0293ce14b4b0281fR68

Closing this issue as I believe this has been solved and will be released soonβ„’.

from child-process.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    πŸ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. πŸ“ŠπŸ“ˆπŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❀️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.