Socket error handling #819

sirsnyder · 2018-02-02T23:32:39Z

The return type of the following methods had to be removed from zend arg info, because these methods can return bool(false) in case of an error:

connect()
read()
write()
send()
select()

The two methods recvfrom() and sendto() can also return bool(false) in case of an error, but there were no explicit return types defined.

Each Socket object holds his own error state now. To query and clear the error state, the two new methods getLastError() and clearError() were added. Method getLastError() differs from the PHP equivalent socket_last_error() by one param. Param clear was added to query and clear error state in one single method call. The param is optional and false by default.

I've tried to mimic the PHP socket error behavior(without warnings) with a view to blocking/non-blocking mode as far as possible. Socket::select() should also not throw exceptions in case of an error. But this method is static and there is no global Socket error state. Therefore a optional param error was added to Socket::select() to retrieve a possible error.

Two -win32 socket tests where removed. @dktapps could you test the both socket-sentto-recvfrom- tests on windows, please?

…nd clearError()

dktapps

Tests are working fine (IPv6 needs a fix in #820 ), I haven't tested any added functionality yet.

dktapps · 2018-02-03T10:25:11Z

src/socket.c

+	int eno = php_socket_errno(); \
+	(socket)->error = eno; \
+	PTHREADS_HANDLE_SOCKET_ERROR(eno); \
+})


I'm not sure about other compilers, but this doesn't register as valid with MSVC (fails to compile). A do {} while(0) works.

dktapps · 2018-02-03T10:47:43Z

classes/socket.h

@@ -66,22 +69,22 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(Socket_listen, 0, 1, _IS_BOOL, 0)
 	ZEND_ARG_TYPE_INFO(0, backlog, IS_LONG, 0)
 ZEND_END_ARG_INFO()

-ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(Socket_connect, 0, 2, _IS_BOOL, 0)
+ZEND_BEGIN_ARG_INFO_EX(Socket_connect, 0, 0, 2)


this doesn't look like it needs changes?

dktapps · 2018-02-03T10:48:59Z

classes/socket.h

@@ -111,18 +114,23 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(Socket_getHost, 0, 0, IS_ARRAY, 0)
 	ZEND_ARG_TYPE_INFO(0, port, _IS_BOOL, 0)
 ZEND_END_ARG_INFO()

-ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(Socket_select, 0, 3, IS_LONG, 0)
+ZEND_BEGIN_ARG_INFO_EX(Socket_select, 0, 0, 4)


In the code the sec parameter still appears to be optional, so the required argcount should be 3.

dktapps · 2018-02-03T10:52:15Z

classes/socket.h

@@ -215,7 +225,7 @@ PHP_METHOD(Socket, listen) {
 	pthreads_socket_listen(getThis(), backlog, return_value);
 } /* }}} */

-/* {{{ proto Socket Socket::accept([string class = self::class]) */
+/* {{{ proto Socket/bool Socket::accept([string class = self::class]) */


the separator ought to be |.

dktapps · 2018-02-03T10:56:39Z

src/socket.c

+	if(threaded->store.sock->error == SUCCESS) {
+		RETURN_FALSE;
+	}
+	ZVAL_LONG(return_value, threaded->store.sock->error);


the RETVAL_LONG macro could be used here

tpunt · 2018-02-03T10:58:57Z

src/socket.c

 	fd_set rfds, wfds, efds;
 	php_socket_t mfd = 0;
 	int result = SUCCESS, sets = 0;
 	struct timeval  tv;

+	zval_dtor(errorno);


This should be zval_ptr_dtor, otherwise it will leak on cyclic references.

Additionally this will segfault if the errorno parameter is not given.

I've copied this part from

pthreads/src/socket.c

Line 775 in d32079f

zval_dtor(port);

. Is zval_dtor in pthreads_socket_recvfrom also faulty?

It looks as though all three variables are not being properly freed:

pthreads/src/socket.c

Lines 773 to 775 in d32079f

zval_dtor(buffer);

zval_dtor(name);

zval_dtor(port);

They should all be using zval_ptr_dtor here (and elsewhere in pthreads_socket_recvfrom).

Ok. zval_dtor was replaced by zval_ptr_dtor. Maybe you could have a look at the original implementation of socket_recvfrom https://github.com/php/php-src/blob/master/ext/sockets/sockets.c#L1771 which uses zval_dtor too.

Ok, I'll have a look into this, thanks.

tpunt · 2018-02-03T10:59:50Z

src/socket.c

-	if (eno != SUCCESS) { \
-		if (estr) { \
-			efree(estr); \
+#define PTHREADS_HANDLE_SOCKET_ERROR(eno) do { \


Conventionally, parentheses should be placed around eno in the macro body.

Parentheses added, should be fixed

sirsnyder · 2018-02-04T00:22:35Z

Some remarks where fixed, thx @dktapps and @tpunt. Please have a look at it again.

dktapps · 2018-02-04T09:03:35Z

src/socket.c

@@ -947,7 +947,7 @@ void pthreads_socket_get_last_error(zval *object, zend_bool clear, zval *return_
 	if(threaded->store.sock->error == SUCCESS) {
 		RETURN_FALSE;
 	}
-	ZVAL_LONG(return_value, threaded->store.sock->error);
+	RETURN_LONG(threaded->store.sock->error);


this will not work as expected (the block below won't be executed), did you mean RETVAL_LONG?

Yep, you're right. RETURN_LONG contains a return;. My fault.

dktapps · 2018-02-04T09:05:18Z

@sirsnyder from a non-technical perspective I don't see anything immediately wrong with it, except for Socket::select() will crash if the error reference parameter isn't specified.

sirsnyder · 2018-02-04T15:51:00Z

@dktapps Socket::select() won't crash anymore without error specified. I've added a null pointer check. @dktapps @tpunt Do you see any more issues? Otherwise I would like to merge this PR.

tpunt · 2018-02-04T21:18:30Z

src/socket.c

 		} \
+		return; \


Won't this return statement prevent the return_value from being set to false later on?

pthreads/src/socket.c

Lines 741 to 743 in f15c95e

PTHREADS_HANDLE_SOCKET_ERROR(eno);

RETURN_FALSE;

Also in the PTHREADS_SOCKET_ERROR call sites.

In case of an exception, there is no need to set a return value or not?

Quite true. I overlooked that part...

tpunt · 2018-02-04T21:30:53Z

I've had another look through the code, and cannot see anything immediately wrong with it (other than the couple of comments I left). Assuming @dktapps doesn't notice any further problems, then feel free to merge. Also, more tests around the sockets functionality (testing both success and error paths) would be good.

tpunt · 2018-02-04T21:32:44Z

src/socket.c

-		if (estr) { \
-			efree(estr); \
+#define PTHREADS_HANDLE_SOCKET_ERROR(eno) do { \
+	if ((eno) != EAGAIN && (eno) != EWOULDBLOCK && (eno) != EINPROGRESS && (eno) != SOCK_EINVAL) { \


EWOULDBLOCK was written to be conditionally used lower down. Will it always be available? If not, then it should be conditionally used here too.

On modern unix systems with GNU C library EWOULDBLOCK is always available with same value of EAGAIN. On modern windows systems EWOULDBLOCK is available but Microsoft do not want to guarantee EWOULDBLOCK. I've changed that. If error no EWOULDBLOCK doesn't exist on win32, it will be defined.

sirsnyder · 2018-02-05T00:04:13Z

@tpunt more tests will come

sirsnyder added 3 commits February 2, 2018 16:59

Extended error handling in none-blocking mode, added getLastError() a…

ef3f0b0

…nd clearError()

Merged master into this branch

7497def

small clean up

2ea86b4

dktapps reviewed Feb 3, 2018

View reviewed changes

tpunt reviewed Feb 3, 2018

View reviewed changes

dktapps and others added 2 commits February 4, 2018 00:56

Fixed IPv6 tests not running when sockets is not compiled

1c055ee

Fixed some remarks

dcbb8ce

dktapps reviewed Feb 4, 2018

View reviewed changes

check null pointer / RETVAL_LONG instead of RETURN_LONG

f15c95e

tpunt reviewed Feb 4, 2018

View reviewed changes

Define EWOULDBLOCK if not defined on win32

98d6085

sirsnyder merged commit b508e06 into master Feb 9, 2018

sirsnyder deleted the socket_error_handling branch February 9, 2018 12:47

This was referenced Feb 9, 2018

socket_last_error() equivalent API #810

Closed

Socket::recvfrom() throws an exception in non-blocking mode when there is no data to be read #809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Socket error handling #819

Socket error handling #819

sirsnyder commented Feb 2, 2018 •

edited

Loading

dktapps left a comment

dktapps Feb 3, 2018 •

edited

Loading

dktapps Feb 3, 2018

dktapps Feb 3, 2018

dktapps Feb 3, 2018

dktapps Feb 3, 2018

tpunt Feb 3, 2018

dktapps Feb 3, 2018

sirsnyder Feb 4, 2018

tpunt Feb 4, 2018

sirsnyder Feb 4, 2018

tpunt Feb 4, 2018

tpunt Feb 3, 2018

sirsnyder Feb 4, 2018

sirsnyder commented Feb 4, 2018

dktapps Feb 4, 2018 •

edited

Loading

sirsnyder Feb 4, 2018

dktapps commented Feb 4, 2018

sirsnyder commented Feb 4, 2018 •

edited

Loading

tpunt Feb 4, 2018 •

edited

Loading

tpunt Feb 4, 2018

sirsnyder Feb 4, 2018

tpunt Feb 5, 2018

tpunt commented Feb 4, 2018 •

edited

Loading

tpunt Feb 4, 2018

sirsnyder Feb 5, 2018

sirsnyder commented Feb 5, 2018

Socket error handling #819

Socket error handling #819

Conversation

sirsnyder commented Feb 2, 2018 • edited Loading

dktapps left a comment

Choose a reason for hiding this comment

dktapps Feb 3, 2018 • edited Loading

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 commented Feb 4, 2018

dktapps Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dktapps commented Feb 4, 2018

sirsnyder commented Feb 4, 2018 • edited Loading

tpunt Feb 4, 2018 • edited Loading

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 commented Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirsnyder commented Feb 5, 2018

sirsnyder commented Feb 2, 2018 •

edited

Loading

dktapps Feb 3, 2018 •

edited

Loading

dktapps Feb 4, 2018 •

edited

Loading

sirsnyder commented Feb 4, 2018 •

edited

Loading

tpunt Feb 4, 2018 •

edited

Loading

tpunt commented Feb 4, 2018 •

edited

Loading