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

Allow defining socket options #46

Closed
wants to merge 4 commits into from
Closed

Conversation

skydiablo
Copy link

No description provided.

@skydiablo
Copy link
Author

facepalm, any hints how to solve the PHP 5.3 problem? btw, is anyone using PHP 5.3 anymore???? :S please NOT!

@jsor
Copy link
Member

jsor commented Mar 4, 2022

The usual approach is, to make the method public and mark it @internal, for example like here:

https://github.com/reactphp/stream/blob/9f7c59c7550d080e9a016e2e722f6b3830763528/src/WritableResourceStream.php#L115-L116

@skydiablo
Copy link
Author

thats ugly, sad...

@WyriHaximus
Copy link
Member

It is :(

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to test this?

composer.json Outdated Show resolved Hide resolved
@skydiablo
Copy link
Author

whats cracking?

@SimonFrings
Copy link
Member

Hey @skydiablo, thanks for the ping 👍

Before going into detail by reviewing the files changed of this pull request, I want to take the time and talk about the "why" behind this pull request. What was the reason that you came across this and what problem does it solve. An answer to this can help me understand, if common use cases are in need of this feature, and will ultimately decide if this should be part of the project.

Interested in your input on this :)

@skydiablo
Copy link
Author

my current project https://codeberg.org/volker.von.hoesslin/reactphp-dhcp-server needs this to define a interface.

@skydiablo
Copy link
Author

@SimonFrings I really don't want to be rude or pushy, but do you need any more input? Can I offer further assistance in merging the PR?

@SimonFrings
Copy link
Member

@skydiablo I used the time to talk with @clue about this pull request. The conversation above already touches on the impact with ext-sockets and my thoughts are: the more we rely on extensions, the less people can use this project.

I don't think this is a bad idea at all, but I'm not sure if reactphp/datagram is the right place to set these socket options. This decision always has an impact on the larger ecosystem, so perhaps it would make more sense to follow a similar solution as @clue found in https://github.com/clue/reactphp-multicast and set the socket options inside the project itself (looking at the createReceiver() method here).

What do you think about this approach?

@skydiablo
Copy link
Author

Hmm, so the code is basically already built in a way that no dependencies are required. If the prerequisites are not met, the feature simply won't function. Alternatively, we could optionally pass a callback to the createServer or createClient function, which would receive the raw socket object for further external processing. Another option is to expose the raw socket object from the React\Datagram\Socket class using a getter.

@clue
Copy link
Member

clue commented Jul 1, 2023

@skydiablo Thanks for filing this PR and sparking this discussion! I understand where you're coming from and agree that being able to set the low-level socket settings would be useful for a number of projects.

Your suggested changes appear reasonable on first sight, but I'm still not sure how I should feel about the fact that this extension may or may not be available and as a consequence the new API may or may not work. Besides the implementation, we'd also have to come up with a good documentation to describe this behavior and a full test suite that covers these code branches in detail. None of this would likely be required for your use case alone, but we'd also have to take the bigger impact on the ecosystem in mind here, so this is definitely something that needs to be covered as part of new feature suggestions like this.

I've discussed this feature before with @SimonFrings and we've come to the conclusion that a solution like https://github.com/clue/reactphp-multicast/blob/master/src/Factory.php#L106 suggested above might perhaps be a worthwhile alternative:

function create(): \React\Datagram\SocketInterface
{
    $stream = @stream_socket_server('udp://0.0.0.0:0', $errno, $errstr, STREAM_SERVER_BIND);
    if ($stream === false) {
        throw new RuntimeException('Unable to create socket: ' . $errstr, $errno);
    }
    
    $socket = socket_import_stream($stream);
    if ($stream === false) {
        throw new RuntimeException('Unable to access underlying socket resource');
    }
    
    // allow multiple processes to bind to the same address
    $ret = socket_set_option($socket, SOL_SOCKET, SO_REUSEADDR, 1);
    if ($ret === false) {
        throw new RuntimeException('Unable to enable SO_REUSEADDR');
    }
    
    return new \React\Datagram\Socket(Loop::get(), $stream);
}

Would be interested to hear your thoughts on this, is this something you've considered before?

@skydiablo
Copy link
Author

hmmm, i can not see how will this help to my usecase? how can i assign me needed custom options, like:

array(
   SOL_SOCKET => array( 
     SO_REUSEADDR => 1,
     SO_BROADCAST' => 1,
     SO_BINDTODEVICE => 'eth0',
  ),
)

there is no chance to reconfig the raw-socket.

@clue
Copy link
Member

clue commented Jul 2, 2023

@skydiablo I'm not sure I follow, the code above includes a socket_set_option($socket, SOL_SOCKET, SO_REUSEADDR, 1), so unless I'm missing something, this seems to be very similar what you're trying achieve? Have you tried similar logic for your socket options? What problem are you seeing exactly?

@skydiablo
Copy link
Author

skydiablo commented Jul 4, 2023

I need to set these two Options for my DHCP Server implentation:

SO_BROADCAST' => 1,
SO_BINDTODEVICE => 'eth0',

So, how can i assign this from the outside of this lib?

@skydiablo
Copy link
Author

maybe just allow to access to the raw-socket in the \React\Datagram\Socket Class via a simple getter?

@SimonFrings
Copy link
Member

I need to set these two Options for my DHCP Server implentation:

SO_BROADCAST' => 1,
SO_BINDTODEVICE => 'eth0',

So, how can i assign this from the outside of this lib?

@skydiablo From what I am getting out of the conversation with @clue I think you just have to do the following:

<?php

use React\Datagram\Socket as DatagramSocket;
use React\EventLoop\Loop;

class Foo
{
    function create(): \React\Datagram\SocketInterface
    {
        $stream = @stream_socket_server('udp://0.0.0.0:0', $errno, $errstr, STREAM_SERVER_BIND);
        if ($stream === false) {
            throw new RuntimeException('Unable to create socket: ' . $errstr, $errno);
        }
        
        $socket = socket_import_stream($stream);
        if ($stream === false) {
            throw new RuntimeException('Unable to access underlying socket resource');
        }

        $ret = socket_set_option($socket, SOL_SOCKET, SO_REUSEADDR, 1);
        if ($ret === false) {
            throw new RuntimeException('Unable to enable SO_REUSEADDR');
        }
        
        $ret = socket_set_option($socket, SOL_SOCKET, SO_BROADCAST, 1);
        if ($ret === false) {
            throw new RuntimeException('Unable to enable SO_BROADCAST');
        }

        $ret = socket_set_option($socket, SOL_SOCKET, SO_BINDTODEVICE, 'eth0');
        if ($ret === false) {
            throw new RuntimeException('Unable to enable SO_BINDTODEVICE');
        }
        
        return new DatagramSocket(Loop::get(), $stream);
    }
}

Have you already tried this out? If this isn't what you're looking for, perhaps it makes sense to get together in a quick Zoom call and go over this together with @clue sometime next week. We are currently in the midst of preparing the ReactPHP birthday, which will take place next Tuesday, so we won't be available until the 13th of July. WDYT?

@skydiablo
Copy link
Author

Oh my God! I was so focused on the factory and didn't realize that I could use the constructor directly (facepalm). Thank you for the eye-opener. Of course, it would have been nice to be able to leverage the benefits of the factory, but I'll just write my own factory and be done with it! Thank you! And sorry for keeping you busy with the topic. Nevertheless, thank you for your work. I love ReactPHP, as you might have noticed. Now I'm trying to do everything with PHP, and it's working really well thanks to ReactPHP!

@skydiablo skydiablo closed this Jul 7, 2023
@SimonFrings
Copy link
Member

Thank you! And sorry for keeping you busy with the topic.

@skydiablo You're welcome, and don't worry about it, we're always happy to help!

I love ReactPHP, as you might have noticed. Now I'm trying to do everything with PHP, and it's working really well thanks to ReactPHP!

Yes, I have noticed that ^^, glad you like the project and really appreciate the kind words ❤️

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

Successfully merging this pull request may close these issues.

5 participants