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 setting socket/tcp options for server sockets. #10

Closed
wants to merge 4 commits into from

Conversation

timgws
Copy link

@timgws timgws commented Sep 18, 2014

This patch will enable users to set options such as SO_REUSEADDR and TCP_NODELAY on new sockets that are created before being passed to the connection emitter.

nodejs by default sets TCP_NODELAY and SO_REUSEADDR to true. I wanted an easy way to replicate this behaviour in my PHP application, so I added the setTCPOption and setSocketOption functions to enable these options to be set.


public function setOption($option_level, $option_name, $option_value)
{
if (!is_array($this->options[$option_level]))
Copy link
Member

Choose a reason for hiding this comment

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

Wrap in parenthesis please

@cboden
Copy link
Member

cboden commented May 10, 2015

I like the API and the feature.

One more thing I'd like to see is error handling. socket_set_option returns false if the option can't be applied. Could you check the return type and emit and error if that happens?

@clue
Copy link
Member

clue commented Jun 2, 2015

Thanks for the PR @timgws and the elaborate README!

I'm a bit surprised this appears to use socket_set_option() (from ext-sockets) on stream socket resources. Could you provide some info on which platforms has this been tested on?

@barrylb
Copy link

barrylb commented Dec 16, 2015

I think you'll find it is necessary to do:
$real_socket = socket_import_stream($socket);
socket_set_option($real_socket, ...);
(on Windows at least you can't just call socket_set_option on a stream resource)

@clue
Copy link
Member

clue commented Jan 27, 2017

Thanks for your effort and filing this PR, but unfortunately we've never been able to get this to work properly.

I believe some of this may have been resolved via #64 in the meantime.

As such, I'm closing this due to a lack of feedback for now. If you feel this is still relevant, please comment on this and we can reopen this 👍

@clue clue closed this Jan 27, 2017
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.

4 participants