Skip to content
This repository has been archived by the owner on Sep 20, 2021. It is now read-only.

Fix node remote address #50

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix node remote address #50

wants to merge 2 commits into from

Conversation

Pierozi
Copy link
Member

@Pierozi Pierozi commented Mar 23, 2017

This PR fix the bug related to the remote address of a connection.

  • As the Server handle multiple connections, it must keep the remote of each node.

@Pierozi Pierozi self-assigned this Mar 23, 2017
@Hywan Hywan assigned Hywan and unassigned Pierozi Mar 23, 2017
Hywan
Hywan suggested changes Mar 23, 2017
View reviewed changes
Connection/Connection.php Outdated
@@ -650,7 +656,14 @@ public function isRemoteAddressConsidered()
*/
public function getRemoteAddress()
{
return $this->_remoteAddress;
$stream = $this->getStream();
$streamId = (int) $stream;
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure this is unique? Maybe spl_object_hash would be a better alternative?

Copy link
Member Author

Choose a reason for hiding this comment

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

Stream is a php resource, and the integer attributed to a php resource is unique within the same process execution. (even through a fork)

Connection/Connection.php Outdated
*
* @var string
*/
protected $_remoteAddress = null;
protected $_remotesAddress = [];
Copy link
Member

Choose a reason for hiding this comment

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

Could we store this information in a node instead? Open question.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's something possible, maybe much appropriate, I need to test if this may have issues related to a pool of nodes.

@Hywan
Copy link
Member

Hywan commented Apr 24, 2017

ping?

@Pierozi Pierozi self-assigned this Apr 27, 2017
@Pierozi
Copy link
Member Author

Pierozi commented Apr 27, 2017

Thanks for the reminder, I will check soon for implement into the node.

@coveralls
Copy link

coveralls commented May 2, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.4%) to 87.427% when pulling 848c531 on Pierozi:fix/node-remote-address into 98199af on hoaproject:master.

@coveralls
Copy link

coveralls commented May 2, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.4%) to 87.427% when pulling 48bc77a on Pierozi:fix/node-remote-address into 98199af on hoaproject:master.

@Pierozi Pierozi requested a review from Hywan May 2, 2017 13:16
@Pierozi
Copy link
Member Author

Pierozi commented May 2, 2017 •
edited by Hywan
Loading

@Hywan I've made change in order to use \Hoa\Socket\Node the design and logic are much better like this.

Hywan
Hywan reviewed May 2, 2017
View reviewed changes
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Are you sure stream_socket_get_name is strictly equivalent to the $address argument of stream_socket_recvfrom?

Connection/Connection.php Outdated
@@ -355,7 +347,8 @@ public function quietAndMute()
*/
public function disconnect()
{
$this->_disconnect = $this->close();
$this->close();
$this->_disconnect = true;
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

close() return void and _disconnect expect to be a boolean

Connection/Connection.php Outdated
return null;
}

return $this->_node->getPeerName();
Copy link
Member

Choose a reason for hiding this comment

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

Why introducing a new vocabulary word (peer vs. remote)? No troll, real question.

Copy link
Member Author

Choose a reason for hiding this comment

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

because the original method behind are stream_socket_get_name and the second argument is called want_peer in php doc

@Pierozi
Copy link
Member Author

Pierozi commented May 2, 2017

@Hywan address on stream_socket_recvfrom isn't working properly, return empty string.

@Pierozi
Copy link
Member Author

Pierozi commented May 2, 2017

I've a concern regarding the implementation of new method getPeerName
if the developer has implemented his own Node, this would break the behavior.

@Pierozi
Copy link
Member Author

Pierozi commented May 24, 2017

Options regarding possible BC break.

  • make getRemoteAddress compatible if Node does not have method getPeerName
    The overhead is minimal like it's one line of code to do the job.
  • Create an Abstract class of Node, and encourage people to extend it for their own node.

@hoaproject/hoackers thought?

@Hywan
Copy link
Member

Hywan commented May 29, 2017

There is already an abstract node class: Node :-].

I don't like having method_exists checks, this is ugly, slow, and error-prone. Let's add it, or break it with PHP 5.x drop.

@Pierozi
Copy link
Member Author

Pierozi commented May 30, 2017

Vote for Break it 👍

@Pierozi
Copy link
Member Author

Pierozi commented Aug 20, 2017

@Hywan Ping status?, I have another PR that will concerne same piece of code.

@Pierozi Pierozi mentioned this pull request Aug 20, 2017
Open
@Hywan
Copy link
Member

Hywan commented Aug 21, 2017

Now that hoa/option is about to be released, we will start to introduce BC break. I will focus on hoa/socket as soon as possible (we need to release other libraries before, but I can lead my work to hoa/socket first).

@Pierozi Pierozi mentioned this pull request Oct 22, 2017
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Hywan Hywan Hywan requested changes

Assignees

@Hywan Hywan

@Pierozi Pierozi

Labels
bug difficulty: hard in progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Pierozi @Hywan @coveralls