-
-
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.
-
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.
-
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_.
-
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.
-
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...
-
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
-
-
[Network] add stream socket class
Review Request #7 — Created Oct. 25, 2012 and discarded
Information | |
---|---|
olivier | |
Lunr | |
personal/olivier/stream | |
12249 | |
Reviewers | |
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 |
Change Summary:
add properties timeout_seconds and timeout_micros for stream socket API as requested. method set_timeout modified : now part of the fluid interface boolean returning methods name changed to is_readable, is_writable, is_exception_thrown... test case updated for basic test of the added properties, timeout test case file removed, set timeout method test case to write
-
-
-
system/libraries/network/class.streamsocket.inc.php (Diff revision 2) imagine how easy this would be just using is_boolean()...
-
tests/system/libraries/network/class.streamsocket.base.test.php (Diff revision 2) wrong comment, btw did I tell you about booleans before?
-
tests/system/libraries/network/class.streamsocket.state.test.php (Diff revision 2) Comment is exactly the same as in previous function, but the test is different... I don't understand it
Change Summary:
change made according to Felipe remarks: -blocking type is now boolean, unit test modified and passed comments changed for two unit tests
Diff: |
Revision 3 (+1487 -10)
|
---|
-
-
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.
Change Summary:
Comment fixes
Diff: |
Revision 4 (+1487 -10)
|
---|
-
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();
-
-
system/libraries/network/class.streamsocket.inc.php (Diff revision 4) No shorthand syntax for if in Lunr. Use brackets. Always.
-
system/libraries/network/class.streamsocket.inc.php (Diff revision 4) I think wrapper is confusing here. Consider a different name
-
-
-
system/libraries/network/class.streamsocket.inc.php (Diff revision 4) no need for surrounding . Don't use single character variables. Stay descriptive
-
-
system/libraries/network/class.streamsocket.inc.php (Diff revision 4) Is this an optional or obligatory call?
Change Summary:
set_timeout method split in two, changed method now uses the properties timeout_seconds and timeout_micros unit test passed
Diff: |
Revision 5 (+1498 -10)
|
---|
-
-
system/libraries/network/class.streamsocket.inc.php (Diff revisions 3 - 5) be descriptive. Nobody can guess "micros"
-
system/libraries/network/class.streamsocket.inc.php (Diff revisions 3 - 5) reset timeout values after use
-
Change Summary:
fixed early exit purposes, add comment for open method precising it's an optional call. tests updated and passed as timeout_micros has become timeout_microseconds
Diff: |
Revision 6 (+1511 -10)
|
---|
Change Summary:
improve early exit statements
Diff: |
Revision 7 (+1515 -10)
|
---|
Change Summary:
Complete test case for state methods, coverage 100%
Diff: |
Revision 8 (+1851 -10)
|
---|
Change Summary:
correction made to package declaration changed from Core to Network
Diff: |
Revision 9 (+1851 -10)
|
---|
Change Summary:
add abstract method read_all()
Diff: |
Revision 10 (+1858 -10)
|
---|
-
-
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.
-
system/libraries/network/class.streamsocket.inc.php (Diff revision 10) set_timeout_in_seconds or set_seconds_timeout
-
system/libraries/network/class.streamsocket.inc.php (Diff revision 10) set_timeout_in_microseconds or set_microseconds_timeout
-
system/libraries/network/class.streamsocket.inc.php (Diff revision 10) I don't think this is necessary. What did you want to achieve with this?
-
system/libraries/network/class.streamsocket.inc.php (Diff revision 10) What's the difference between read() and read_all()?