Rework of sockets to mimic php-src sockets API #839

sirsnyder · 2018-02-19T23:58:23Z

Macros PTHREADS_SOCKET_ERROR and PTHREADS_HANDLE_SOCKET_ERROR extended by msg and eno for detailed exceptions
Extended error handling
Bug fix in Socket::getOption()
Param sec of Socket::select() is mandatory. Valid values are 0, > 0 and null.
Param port of Socket::bind() is optional, default value is 0.
Param backlog of Socket::listen() ist optional, default value is 0.
Added serveral tests

sirsnyder · 2018-02-20T11:34:14Z

@dktapps @tpunt please review this PR. Looks like Appveyor has problems with phpsdk_deps, don't know why. (Edit: https://twitter.com/nikita_ppv/status/965905195019526145)

After this PR has been merged, more additions and tests has to come. PR #828, extension of get/setOptions(), importStream()/exportSocket() and getErrorMessage() are in preparation. Later on the IPV6 stuff.

tpunt

I've given it a quick review.

tpunt · 2018-02-20T09:02:13Z

classes/socket.h

@@ -183,7 +183,7 @@ PHP_METHOD(Socket, setOption) {
 	zend_long value = 0;

 	if (zend_parse_parameters(ZEND_NUM_ARGS(), "lll", &level, &name, &value) != SUCCESS) {
-		return;
+		RETURN_FALSE;


I understand why you've changed this to return false, but conventionally, when the ZPP API fails to parse the input, it will return null (this null return is usually ignored in the docs, since only the correct usage is documented). The socket_set_option function does the same internally. This leads to the following output:

tpunt:~ tpunt$ php -nr 'var_dump(socket_set_option());'

Warning: socket_set_option() expects exactly 4 parameters, 0 given in Command line code on line 1
NULL

For parity with other internal functions, it would likely be best to keep these as returning null, too.

socket_set_option has no explicit return type defined by ZEND_BEGIN_ARG_INFO_EX, but Socket::setOption uses ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX. Without return false, this leads to the following output:

$socket = new Socket(\Socket::AF_INET, \Socket::SOCK_STREAM, \Socket::SOL_TCP); $socket->getOption();

Warning: Socket::getOption() expects exactly 2 parameters, 0 given in ...

Fatal error: Return value of Socket::getOption() must be of the type integer, null returned in Unknown on line 0

Hmm, I do see the problem. This is also why the return type information macros are not particularly pervasive in php-src. I'm not so concerned what solution is chosen here (current solution looks fine).

tpunt · 2018-02-20T11:39:42Z

classes/socket.h

 	zend_long usec = 0;

-	if (zend_parse_parameters(ZEND_NUM_ARGS(), "a/!a/!a/!|llz/", &read, &write, &except, &sec, &usec, &errorno) != SUCCESS) {
+	if (zend_parse_parameters(ZEND_NUM_ARGS(), "a/!a/!a/!z/!|lz/", &read, &write, &except, &sec, &usec, &errorno) != SUCCESS) {


Separation of the sec parameter zval is unnecessary.

sec needs to be nullable. Is that possible with zend_long ?

Since the parameter isn't being typed, then it is perfectly valid to pass in null to it. Separation is only needed for by-reference passing.

I'm going to remove the separation. What would you recommend, keeping sec as zval or moving back to zend_long?

I think I messed up something. zend_long is just an int32_t, so it won't be possible to handle null with it, right?

That's correct. The exclamation mark will enable for it to be nullable, but you won't be able to differentiate between passing null or 0 to the parameter. So it must be kept as a zval.

tpunt · 2018-02-20T11:45:36Z

src/socket.c

-	PTHREADS_HANDLE_SOCKET_ERROR(eno); \
+#define PTHREADS_SOCKET_ERROR(socket, msg, eno) do { \
+	(socket)->error = (eno); \
+	PTHREADS_HANDLE_SOCKET_ERROR((eno), (msg)); \


Wrapping these arguments in parentheses isn't actually necessary (since they are being expanded inside of another macro, which should be wrapping them anyway).

Ok, I'm not familar with arguments in macros. Will change that.

tpunt · 2018-02-20T11:51:33Z

src/socket.c

@@ -125,12 +123,12 @@ void pthreads_socket_set_option(zval *object, zend_long level, zend_long name, z
 void pthreads_socket_get_option(zval *object, zend_long level, zend_long name, zval *return_value) {
 	pthreads_object_t *threaded =
 		PTHREADS_FETCH_FROM(Z_OBJ_P(object));
-	socklen_t unused;
+	socklen_t unused = sizeof(int);


It's probably not needed, but if we are going to initialise this value, then it should be sizeof(zend_long), since that's the size used by an integer zval.

If unused is not initialized, Socket::getOption() returns always 0 for the first call. int was copied from php-src, will change it to zend_long.

tpunt · 2018-02-20T11:52:35Z

src/socket.c

@@ -235,6 +244,7 @@ void pthreads_socket_bind(zval *object, zend_string *address, zend_long port, zv
 		PTHREADS_FETCH_FROM(Z_OBJ_P(object));
 	php_sockaddr_storage  sa_storage = {0};
 	struct sockaddr     *sock_type = (struct sockaddr*) &sa_storage;
+	zend_long			retval = 0;


nit: indentation (not that it was particularly consistent before this PR)

tpunt · 2018-02-20T11:54:16Z

src/socket.c

 		case AF_INET: {
 			struct sockaddr_in *sa = (struct sockaddr_in *) sock_type;

 			sa->sin_family = AF_INET;
 			sa->sin_port = htons((unsigned short) port);

 			if (!pthreads_socket_set_inet_addr(threaded->store.sock, sa, address)) {
-				/* throw */


Is there any reason why we do not throw here? Same as below.

The corresponding exception will be thrown in pthreads_socket_set_inet_addr(). We just have to return at this point. Same for pthreads_socket_set_inet6_addr().

tpunt · 2018-02-20T11:57:07Z

src/socket.c

-		default:
-			return;
-			/* throw */
+	if (retval != 0) {


nit: comparing to SUCCESS would be semantically clearer

Yes, that would be clearer

tpunt · 2018-02-20T12:03:59Z

src/socket.c

+	/* overflow check */
+	if ((length + 1) < 2) {
+		RETURN_FALSE;
+	}


zend_long is a signed type, and overflow on signed types is undefined behaviour according to the C standard. PHP has a ZEND_LONG_MAX macro that could be used instead.

Maybe the term overflow check is not right here. I've copied this snippet from php-src and think it is used to ensure length is not negativ and at least 1. Because length <= 0would not make sense.

That sounds correct - perhaps the comment should be removed and the condition updated to length < 1.

tpunt · 2018-02-20T12:07:18Z

src/socket.c

-	if (sec || usec) {
+
+	/* If seconds is not set to null, build the timeval, else we wait indefinitely */
+	if (sec != NULL) {


From what I can tell, sec can never be NULL, since it is not an optional parameter (the same check in php-src looks superfluous).

sec needs to be nullable. The following applies to Socket::select() and socket_select()

sec < 0 - Invalid argument

sec = 0 - returns immediately

sec > 0 - waits definitely

sec = null - waits indefinitely

Right, I see. I didn't realise sec would be NULL (C null) if a PHP null was passed in to it (I assumed it would have been Z_TYPE_P(sec) == IS_NULL). My mistake.

tpunt · 2018-02-20T12:10:07Z

src/socket.c

+		tv_p = &tv;
+
+		if (sec == &tmp) {
+			zval_dtor(&tmp);


Longs are not reference counted, and so there's no need to destroy them (the same operation in php-src looks superfluous - probably outdated code from PHP 5, when every type was reference counted).

Good to know. I've removed the zval_dtor call.

sirsnyder · 2018-02-22T11:06:54Z

@tpunt thanks for your review so far, any further notes?

tpunt · 2018-02-22T11:36:57Z

src/socket.c

 		} break;
 #endif
 	}
+
+	if (retval != 0) {


nit: retval != SUCCESS

I've missed this line, sorry!

tpunt

@sirsnyder This looks fine to me now. Feel free to squash and merge it.

dktapps · 2018-02-22T16:20:29Z

examples/stub.php


    public function accept($class = self::class){}

    public function connect(string $host, int $port = 0) : bool{}

-    public static function select(array &$read, array &$write, array &$except, int $sec = 0, int $usec = 0, int &$error = null){}
+    public static function select(array &$read, array &$write, array &$except, int $sec, int $usec = 0, int &$error = null){}


This should be ?int $sec

Good catch, do you see any other issues?

Nothing off the hop, this is the only thing I noticed.

dktapps

I'm experiencing various test failures on Windows as noted in the comments.

dktapps · 2018-02-22T16:49:42Z

tests/socket-select.phpt

+int(0)
+int(0)
+bool(false)
+int(22)


This test does not work as expected on Windows (I'm getting all zeros, no errors) - not sure if this is a Windows-specific bug or what, haven't investigated in-depth yet.

On win32, a negative timeout seems to be handled like a zero timeout, without any error.

dktapps · 2018-02-22T16:50:53Z

tests/socket-getoption.phpt

+    } catch(Throwable $throwable) {
+        var_dump($throwable->getMessage());
+    }
+    var_dump($socket->getOption(SOL_SOCKET, SO_REUSEADDR));


use of global ext/socket constants causes this test to fail without ext/sockets

dktapps · 2018-02-22T16:52:11Z

tests/socket-connect-error.phpt

+string(%d) "Argument 1 passed to Socket::__construct() must be of the type integer, array given"
+string(%d) "Unable to create socket (%d): Address family not supported by protocol"


This error message is different on Windows:

005+ string(96) "Unable to create socket (10047): An address incompatible with the requested protocol was used. 005- string(%d) "Unable to create socket (%d): Address family not supported by protocol" 006+ "

Note the trailing newline as well (as seen before with Windows sockets).

dktapps · 2018-02-22T16:53:09Z

tests/socket-setoption.phpt

+        var_dump($throwable->getMessage());
+    }
+    var_dump($socket->setOption(SOL_SOCKET, SO_REUSEADDR, 1));
+    var_dump($socket->getOption(SOL_SOCKET, SO_REUSEADDR));


same again here - use of global ext/sockets constants causes this to fail without ext/sockets

dktapps · 2018-02-22T16:54:22Z

tests/socket-getsockname.phpt

+    if (file_exists($address))
+        die('Temporary file socket already exists.');
+
+    $unixSocket = new \Socket(\Socket::AF_UNIX,\Socket::SOCK_STREAM, 0);


This fails on Windows (no UNIX sockets).

Forgot to split the test, fixed.

sirsnyder · 2018-02-22T19:20:15Z

I will start a second branch for fixes

sirsnyder · 2018-02-23T10:44:37Z

Thanks @tpunt and @dktapps for your support!

sirsnyder force-pushed the rework_sockets branch from 7f485cd to 417fdf2 Compare February 20, 2018 11:02

sirsnyder changed the title ~~WIP: Rework of sockets to mimic php-src sockets API~~ Rework of sockets to mimic php-src sockets API Feb 20, 2018

sirsnyder requested a review from tpunt February 20, 2018 11:20

tpunt reviewed Feb 20, 2018

View reviewed changes

tpunt reviewed Feb 22, 2018

View reviewed changes

tpunt approved these changes Feb 22, 2018

View reviewed changes

Rework of sockets to mimic php-src sockets API

26eace5

sirsnyder force-pushed the rework_sockets branch from e9b197f to 26eace5 Compare February 22, 2018 15:29

Merged branch master into rework_sockets

47423ce

sirsnyder merged commit 7c4a62d into master Feb 22, 2018

sirsnyder deleted the rework_sockets branch February 22, 2018 15:56

dktapps reviewed Feb 22, 2018

View reviewed changes

sirsnyder mentioned this pull request Feb 23, 2018

Fixed sockets tests + stub #842

Merged

		string(%d) "Argument 1 passed to Socket::__construct() must be of the type integer, array given"
		string(%d) "Unable to create socket (%d): Address family not supported by protocol"

Rework of sockets to mimic php-src sockets API #839

Rework of sockets to mimic php-src sockets API #839

Conversation

sirsnyder commented Feb 19, 2018 • edited

sirsnyder commented Feb 20, 2018 • edited

tpunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirsnyder Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpunt Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirsnyder commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dktapps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirsnyder commented Feb 22, 2018

sirsnyder commented Feb 23, 2018

sirsnyder commented Feb 19, 2018 •

edited

sirsnyder commented Feb 20, 2018 •

edited

sirsnyder Feb 20, 2018 •

edited

tpunt Feb 20, 2018 •

edited