5
\$\begingroup\$

I am in the process of writing networking code for PHP that is meant to be used in long-running PHP processes. I realize that PHP is supposedly not well suited for long-running processes, however that is not my question, and I already am well aware of the implications. This is meant as an OOP learning exercise for the most part.

My idea is to write a networking class that can be dependency-injected into a protocol-handling class. In the final product, these are meant to be two separate files, that use namespaces properly! I've put them here as namespace-less files:

  • \MyApp\Net\Stream\Client (which implements the interface \MyApp\Net\Generic\Client)
  • \MyApp\Net\SomeProtocol\Client (which implements \MyApp\GenericProtocol\Client, or something another)

The entire structure ends up looking like:

  • \MyApp\Net\Generic\Client
  • \MyApp\Net\Generic\Server
  • \MyApp\Net\Stream\Client (uses newer style stream_* for underlying functionality)
  • \MyApp\Net\Stream\Server
  • \MyApp\Net\Socket\Client (uses old style socket_* for underlying functionality)
  • \MyApp\Net\Socket\Server
  • \MyApp\ProtocolName\Client (gets an implementation of \MyApp\Net\Generic\Client injected into it)

and so on, and so on.

I've commented some of the code that I'm feel most unsure about. In addition - the idea was to add PSR-compatible logging, so mind the dingy echo statements. (This will actually run as-is without any additional code).

The Generic Network Class

class StreamClient {
  private $socket;
  private $status = 'disconnected';
  private $recvBufferSize = 4096;
  private $parent; // Is the "parent" the correct term for this?

  // in reality this would be a generic interface as the type for $parent
  public function __construct(TcpReverseEcho $parent = null) {
    $this->parent = $parent;
  }

  public function setParent(TcpReverseEcho $parent) {
    $this->parent = $parent;
  }

  public function connect($where) {
    if($this->status != 'disconnected') {
      echo " > connect() called while not disconnected...\n";
      return false;
    }

    $this->status = 'connecting';
    $this->socket = @stream_socket_client(
      $where, $errno, $errstr,
      STREAM_CLIENT_ASYNC_CONNECT
    );

    // Even in async mode, sometimes the socket will immediately return false.
    if($this->socket == false) {
      echo " > socket connection failed early.\n";
      $this->status = 'disconnected';
      $this->on_disconnect('connection-attempt-failed', $errno, $errstr);
    } else {
      stream_set_blocking($this->socket, false);
    }
  }

  // Only use disconnect() if you wish to forcibly close the connection from our side
  public function disconnect() {
    $this->status = 'disconnected';
    @fclose($this->socket);
    $this->on_disconnect('disconnect-called');
  }

  public function send($data) {
    $this->on_send($data);

    return fwrite($this->socket, $data);
  }

  public function recv() {
    $data = fread($this->socket, $this->recvBufferSize);
    if(strlen($data) > 0) {
      $this->on_recv($data);
    }
  }

  public function iterate() {
    if($this->status != 'disconnected' && is_resource($this->socket)
      && !feof($this->socket)) {

      echo " > Iteration... \n";
      echo " > The socket looks to be in good condition...\n";

      // The socket was connecting but now is connected..
      if($this->status == 'connecting') {
        $this->status = 'connected';
        $this->on_connect();
      } else {
        $this->recv();
      }
    } else {
      if($this->status == 'connected') {
        echo " > the socket was disconnected from the outside...\n";
        $this->status = 'disconnected';
        $this->on_disconnect();

        // Unsure: Will this ever occur, particularly with
        // non-blocking sockets? I can't get it to happen but I'm
        // only using loopback..
      } elseif($this->status == 'connecting') { //
        echo " > the socket connection failed!\n";
        $this->status = 'disconnected';
        // I think I can still use stream_lasterr() type function here.
        $this->on_disconnect();
      } else {
        echo " > the socket is in a disconnected state...\n";
      }
    }
  }

  // Ability to change the buffer size...
  public function getRecvBufferSize() {
    return $this->recvBufferSize;
  }

  public function setRecvBufferSize($bufferSize = 4096) {
    $this->recvBufferSize = $bufferSize;
  }

  // oes this look too redundant?  This allows us to instead of injecting
  // this class/object into a protocol handler, we could actually instead
  // just extend this class and override these methods  below.  Maybe
  //even make sense to check for $this->parent being..
  // Or is this "wrong?"
  function on_connect() {
    if($this->parent != null) {
      echo " > calling parent on_connect()\n";
      $this->parent->on_connect();
    }
  }

  function on_disconnect($context = null, $errno = null, $errstr = null) {
    if($this->parent != null) {
      echo " > calling parent on_disconnect($context, $errno, $errstr)\n";
      $this->parent->on_disconnect($context, $errno, $errstr);
    }
  }

  function on_send($data) {
    if($this->parent != null) {
      echo " > calling parent on_send(" . trim($data) . "\n";
      $this->parent->on_send($data);
    }
  }

  function on_recv($data) {
    if($this->parent != null) {
      echo " > calling parent on_recv(" . trim($data) . "\n";
      $this->parent->on_recv($data);
    }
  }
}

The Protocol Handler Class

This is our protocol handler, the idea being that we inject the network object above into the protocol handler This particular example is a simple 'reverse echo'. Once the client is connected, it sends back everything the server sends, reversed (so reversed textually and in the normal model, haha, get it...)

class TcpReverseEcho {
  private $net;

  // in reality this would be a generic interface as the type hint
  public function __construct(StreamClient $net) {
    // Unsure: Is this still proper use of dependency injection?
    $net->setParent($this);
    $this->net = $net;
  }

  public function connect() {
    // I'll use netcat -l -p 1234 for the testing server..
    $this->net->connect('tcp://127.0.0.1:1234');
  }

  public function iterate() {
    $this->net->iterate();
  }

  public function on_connect() {
    $this->net->send("A client has connected, sir.\n");;
  }

  public function on_disconnect($context = null, $errno = null, $errstr = null) {
    echo " > Socket disconnected: $context, $errno, $errstr\n";
    exit;
  }

  public function on_send($data) {
    echo " > Protocol Handler Sent: " . trim($data) . "\n";
  }

  public function on_recv($data) {
    $data = strrev($data);
    $this->net->send(trim($data) . "\n");
  }
}

And the glue...

Finally, some glue to hold it all together:

$netClient = new StreamClient();
$protocolClient = new TcpReverseEcho($netClient);
$protocolClient->connect();

while (1) {
  $protocolClient->iterate();
  sleep(1);
}

If you paste this all into one file, in order, it will run. Any advice or feedback, particularly regarding my use of dependency injection and callbacks, would be greatly appre

\$\endgroup\$
  • 2
    \$\begingroup\$ Just a generic PHP practice I've started using: if you're checking specifically against boolean false you should use === instead of the way-looser ==; if you're checking generically against falsey values, use something like !$this->socket instead of $this->socket == false. It's a microoptimization but given the range of values PHP accepts as falsey I've found you can sidestep a lot of type juggling trickiness and code ambiguity by being more specific. \$\endgroup\$ – moberemk Jun 28 '15 at 14:24
  • \$\begingroup\$ I noticed you're mixing socket functions with file/stream functions. There's a range of socket-related functions already. Granted quite a few socket man pages just say "this function is currently undocumented." Any luck just experimenting with those undocumented functions anyway? \$\endgroup\$ – bob-the-destroyer Jul 5 '15 at 0:21
  • \$\begingroup\$ Also, I don't see any cleanup. For example, making sure connections are properly disposed of (or closed at the very least) after no longer needed. If this is a long-running process, then efficiently managing sockets will be crucial otherwise zombie connections will just keep piling up. \$\endgroup\$ – bob-the-destroyer Jul 5 '15 at 0:27
5
+100
\$\begingroup\$

Your code is really good, but there's a few things you could improve:


$errno, $errstr, it's silly to sacrifice readability for a few extra characters:

$errorNumber & $errorString is much better.


Instead of while (1), you should use while (true)


You should avoid directly adding words/n into strings, but rather 'words'.'/n'


Other than that, your code looks clean and well structured, good work!

\$\endgroup\$
5
\$\begingroup\$
class StreamClient {

I would recommend using namespaces or if you are targeting pre-namespace PHP pseudo-namespaces

private $socket;

Marking your variables as private will make it difficult to extend and reuse this class.

// in reality this would be a generic interface as the type for $parent
public function __construct(TcpReverseEcho $parent = null) {
  $this->parent = $parent;
}

public function setParent(TcpReverseEcho $parent) {
  $this->parent = $parent;
}

It looks like you are passing parent is for event notification. A better pattern is to to allow other objects to subscribe to events using the observer pattern

// oes this look too redundant?  This allows us to instead of injecting
// this class/object into a protocol handler, we could actually instead
// just extend this class and override these methods  below.  Maybe
//even make sense to check for $this->parent being..
// Or is this "wrong?"
function on_connect() {
    if($this->parent != null) {
        echo " > calling parent on_connect()\n";
        $this->parent->on_connect();
    }
}

The main thing that stands out as an issue is single repressibility principle and separation of concerns. You classes doesn't have clear repressibility and they are trying to do to much. You need to make StreamClient as dumb as possible. You then inject StreamClient into TcpReverseEcho. The communication should flow from TcpReverseEcho to StreamClient with only responses coming back from method calls and from registered callbacks (events).

You are on the correct track with trying to separate out StreamClient and inject. You need to make a cleaner break.

\$\endgroup\$
  • \$\begingroup\$ I had in the question that in the actual code namespaces are used, they are separate files, -- I have them without the namspacing here simply so they would run within a single file and to reduce the already fairly lengthly code paste. \$\endgroup\$ – A.B. Carroll Jul 2 '15 at 23:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.