[Network] add stream socket class

Review Request #7 — Created Oct. 25, 2012 and discarded

olivier
Lunr
personal/olivier/stream
12249
lunr
Add StreamSocket class to Network namespace.

Abstract class created in system/libraries/network/class.streamclient.inc.php:
-Will be followed by concrete class StreamSocketClient.
-Should be use later if need of a StreamSocketServer class

Unit test case created in tests/system/libraries/network/class.streamsocket.*.test.php
-ant phpunit
-custom test made for sending push notification to APNS
  • 1
  • 0
  • 16
  • 0
  • 17
Description From Last Updated
I'm slightly worried about the naming. The Curl class has a set_option and set_options method. They are similar to the ... pprkut pprkut
pprkut
  1. I can see you spent a lot of time on building a class that's very close to the original stream API. While this is not necessarily a bad thing, it doesn't have to be good either.
    In this case I'm still uncertain which one it is, mostly because an actual use-case is missing (and also because I'm not that familiar with PHP streams)
    What I can see already though is that some things are better served differently:
    
    - A lot of functions take a seconds and microseconds parameter. Considering there is a fluid interface already, those are much better served by a dedicated function call as part of the fluid interface.
    - A lot of methods return TRUE/FALSE based on their return values from the PHP internal methods. Please consider that might not be what users of the API are actually interested in. It's probably worth turning those
      methods into a fluid interface as well and providing error information (in case there is one) via a different method.
    
    I'd also be interested in someone else's input here.
    
    As a note for you for future work. In case you need to abstract away a completely new API, put out a RFC first, without unit tests. If you then have to change the API drastically, you at least didn't waste hours in writing unit tests for calls that get dropped.
    1. So then I must just say that it was made on purpose.
      
      cause the fluid interface has its limits in such case IMHO:
      
      considering this:
      
      $stream->set_context_option(..)
             ->add_flag(...)
             ->set_blocking(...)
             ->set_notification_callback(...)   //Here ends the configuraton of the stream, now let's run it.
             ->open()                           
             ->write(...)
             ->can_be_read(...)                 //Here Three possibilities: the stream did not change (like on an APNS successfull push), no need to try to read it otherwise endless loop...
                                                //If the stream changed, need to read.
                                                //If an error occured, skip to next read to see if write still works.
      
      Then the case is the following:
      no answer  > continue
      answer     > read it but need to closed the connection cause APNS closed it too
      If failure > this can happen if the system call is interrupted by an incoming signal(as the php manual says),
                   can always try to write again.
             
    2. From the APNS apple site:
      
      "You should also retain connections with APNs across multiple notifications. APNs may consider connections that are rapidly and repeatedly established and torn down as a denial-of-service attack. Upon error, APNs closes the connection on which the error occurred."
      
      Then it is safer to keep the connection open and close it and reopen according to the "can_be_read" call.
      
      
    3. As I said, it's hard to review something without an actual use case. The one you gave in your comment is fine, but only one small part of the bigger picture. I wasn't talking about the can_be_read() function at all. If you look at set_timeout(), it returns a boolean, whereas I as a user would expect it to work like the other set_* methods.
      
      As a said note, boolean methods need to start with is_, not with can_ and not with has_.
    4. I agree with Heinz on the seconds and micro senconds thing, I don't think it makes much sense to pass them in every function instead of just setting them as part of the fluid interface.
      I can't see functions returning booleans anymore, everything seems to follow the fluid interface properly.
      For giving my opinion on the function changed and how it's used, I would need to see an specific use case.
    5. For what concerns changed(), it is just a wrapper of stream_select and it is called in the "is_readable", "is_writable" and "is_exception_thrown" methods.
      
      Use case is then:
      
      $stream->is_readable(1);
      Again for APNS, this is useful in order to know the return code if any, and then close the stream cause it is closed on server side.
      
      I already made an implementation of the read and write methods that REOPENS the stream if the handle is not a resource(e.g was closed)
      in the coming StreamSocketClient class.
      
      To go further with the fluid interface, I would submit the following possibility:
      
          -adding methods, part of the fluid interface which would indicate the StreamSocket what to to in case of answer 
           (something was read after a call on is_readable) like "public function close_on_answer/close_on_read{...return $this}"
          
          -coupling this with a observer pattern, like StreamChangeListerner interface.
          
          -adding methods such as listen_to_answer or equivalent (part of the fluid interface)which would indicate the class what to do after sending data on the stream
           by making the stream listen to himself and consequently act on events such as "write"
      	 
          -The boolean returning methods would then be set to protected ("is_readable", "is_writable" and "is_exception_thrown").
           With addition of extra property for the parameters and setters.
      
      In the end, the APNS class would listen to the stream to know the status of the push...
      
      Hope I was clear...
      
      
    6. Whatever you mean with a StreamChangeListener is way out of scope for this review. One small step after the other. The review is already big enough, no need to make it any bigger. KISS
  2. 
      
olivier
Felipe
  1. 
      
  2. why not use a boolean?
    
  3. imagine how easy this would be just using is_boolean()...
  4. wrong comment, btw did I tell you about booleans before?
  5. Comment is exactly the same as in previous function, but the test is different... I don't understand it
    
  6. 
      
Felipe
olivier
pprkut
  1. 
      
  2. system/libraries/network/class.streamsocket.inc.php (Diff revisions 1 - 3)
     
     
     
     
     
     
     
     
     
     
    This change doesn't preserve behavior. It's also completely useless as neither timeout_seconds nor timeout_micros is used anywhere.
    
    Please set second and microsecond values over a proper fluid interface, and remove the unnecessary tv_sec and tv_usec parameters.
  3. 
      
olivier
olivier
  1. Here is a sample that uses StreamSocket client for APNS:
    
    /**
     * Prepare the data for the push
     */
    $token = '186826258b2924d58d75dba38ef7bf411dbcee085f0ac72e53b9c43f4381861b';
    $message = "Viva new APNS from lunr!!!" ;
    $pid = 125; //process Id for apns
    // Formating payload
    $payload = array('aps' => array());
    $payload['aps']['alert'] = $message;
    $json_payload = json_encode($payload);
    
    /**
     * Building and preparing the stream
     */
    $stream = new StreamSocketClient('ssl://gateway.sandbox.push.apple.com:2195', 30);
    
    $stream->set_context_options
    (    array
         ('ssl' => array
                  (  'local_cert' => './statics/pkresources/PushNotificationsTestFull.pem',
                     'passphrase' => 'Clint0n'
                  )
         )
    );
    
    $expiry = time()+120; // 2 minute validity hard coded!
    $msg = chr(1).pack("N",$pid).pack("N",$expiry).pack("n",32).pack('H*',$token).pack("n",strlen($json_payload)).$json_payload;
    
    $stream->add_flag('STREAM_CLIENT_PERSISTENT');
    
    $stream->open();
    
    /**
     * Now the sequance that should be managed by APNS class
     */
    $fwrite = $stream->write($msg); //Consider that write reopens the stream if needed, no need to recall open
    
    if(!$fwrite)
    {
        echo 'Unable to write data<br>';
        $stream->close();
    }
    else
    {   $changed = $stream->is_readable(1);
    
        if(NULL===$changed)
        {
            echo "Failed selecting stream to read.<br>";
        }
        else if($changed === TRUE)
        {
            $error     = unpack('Ccommand/Cstatus_code/Nidentifier', $stream->read(6));
            $command   = $error['command'];
            $status    = $error['status_code'];
            $identifier = $error['identifier'];
            // Of course this will not be within the method push...
            $statusDesc = array(
                    0 => 'No errors encountered',
                    1 => 'Processing error',
                    2 => 'Missing device token',
                    3 => 'Missing topic',
                    4 => 'Missing payload',
                    5 => 'Invalid token size',
                    6 => 'Invalid topic size',
                    7 => 'Invalid payload size',
                    8 => 'Invalid token',
                    255 => 'None (unknown)',
            );
            echo "APNS responded with command($command) status($status) pid($identifier)<br>";
    
            if($status>0)
            {
                $desc = isset($statusDesc[$status])?$statusDesc[$status]: 'Unknown';
                echo "Push FAILED: APNS responded with error for pid($identifier). status($status: $desc)<br>";
                $stream->close();
            }
            else
            {    echo "Push successful: APNS responded with command($command) and status($status: $desc) for pid($identifier)<br>";
            }
        }
        else
        {
            echo "Push successful: stream state did not change, no status code received<br>";
        }
    }
    /**
     * Stream is closed in the class that built it...
     */
    $stream->close();
    
    1. While this is a valid use case, it does not use the fluid interface. This can easily be rewritten as:
      
      $stream = new StreamSocketClient(..);
      $status = $stream->set_context_options($options)
                       ->add_flag(...)
                       ->write($msg);
      
      if ($status === FALSE)
      {
          $this->errno = 123;
          $this->errmsg = "Error";
          return;
      }
      
      ...
  2. 
      
pprkut
  1. 
      
  2. No shorthand syntax for if in Lunr. Use brackets. Always.
  3. I think wrapper is confusing here. Consider a different name
    1. Named like in the php manual.
  4. system/libraries/network/class.streamsocket.inc.php (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
    early exit
  5. system/libraries/network/class.streamsocket.inc.php (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
    early exit
  6. no need for surrounding .
    Don't use single character variables. Stay descriptive
  7. 
      
pprkut
  1. 
      
  2. system/libraries/network/class.streamsocket.inc.php (Diff revision 4)
     
     
     
     
     
     
     
    Is this an optional or obligatory call?
  3. 
      
olivier
pprkut
  1. 
      
  2. be descriptive. Nobody can guess "micros"
    1. I can, it stands for microseconds :) It's kinda easy when you have timeout next to it...
  3. reset timeout values after use
    1. I dont see the point in doing this, timeout can be kept the same for multiple call.
    2. Agree with olivier here, specially if we make them part of the fluid interface
  4. system/libraries/network/class.streamsocket.inc.php (Diff revisions 3 - 5)
     
     
     
    No shorthand if
  5. 
      
olivier
olivier
olivier
olivier
olivier
olivier
pprkut
  1. 
      
  2. system/libraries/network/class.streamsocket.inc.php (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
     
    I'm slightly worried about the naming.
    The Curl class has a set_option and set_options method. They are similar to the set_flag and set_flags methods you defined in the client class. And set_context_option(s) does something different.
    This is rather confusing and should be synchronized.
    1. As you wish but this is made to stay close form the original API and
      to make the user aware of the fact this concerns the context.
    2. The original API is of no concern to our API. That's the whole point of making a wrapper, otherwise we could just skip it and use the low level code directly.
    3. To harmonize I propose the following:
      
      -renaming set_context_option(s) to set_option(s)
      
      the flags are different from the curl class because
      it only concerns integer values spend as string,
      but is not a key/value association.
      
      finding another name is a possibilitie
    4. So, looking a bit deeper, you can drop the $wrapper parameter. AFAICS there can only be one wrapper at a time and you can detect it from the URL you are using.
      The you can rename $option to $key and we have exactly the same interface as in the curl class
  3. set_timeout_in_seconds or set_seconds_timeout
    1. This sounds clear to me.
      
      set_timeout_in_seconds sounds like you want a time out in seconds only by some way
    2. I would go for set_seconds_timeout
  4. set_timeout_in_microseconds or set_microseconds_timeout
    1. same here then
    2. set_microseconds_timeout for me...
  5. I don't think this is necessary. What did you want to achieve with this?
    1. Cause the array given to stream_select are transmitted by reference.
      http://php.net/manual/en/function.stream-select.php
    2. sure, that's why it needs to be stored in a variable. But why do you store it twice?
    3. Right did not notice that...
  6. system/libraries/network/class.streamsocket.inc.php (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    What's the difference between read() and read_all()?
    1. The difference is that the handle is not exposed.
      Then you could have done thing like
      while($read = $stream->read(1024))
      {
      
      }
      
      which works fine but will be dine again and again you simply write
      
      $stream->read_all();
      
      which returns the whole available data testing internally the feof.
      this is a convenience but I think this is relevant for future use of the 
      extending classes.
      
    2. So read() is a special case of read_all()? Then why do a special read() in the first place and not do a read_all() by default?
    3. Because at first the need was to read a special amount of bytes.
      Then the need for read_all came, I don't think this is such a big issue
      to offer many possibilities.
      
      
    4. I think is good to keep both, imagine you want to check the first line of whatever you receive to decide if you want to process it or not, then you would do a read and if it's what you want, a read_all afterwards
    5. So we make a read($length = 0) where length 0 means read all. No?
    6. Seems to be a good alternative, I'll check the possibilities for next review
  7. 
      
Felipe
  1. Waiting for next review... 
  2. 
      
olivier
Review request changed

Status: Discarded

Change Summary:

replaced by http://reviews.lunr.nl/r/36/
Loading...