Conversation
|
@dktapps please review |
sirsnyder
force-pushed
the
socket_shutdown
branch
from
0796637 to
d4c9931
Compare
dktapps
left a comment
dktapps
left a comment
There was a problem hiding this comment.
I think it would be worthwhile adding constants for the shutdown parameters to the Socket class (something like SHUTDOWN_READ, SHUTDOWN_WRITE, SHUTDOWN_BOTH).
tests/socket-shutdown.phpt
Outdated
| $socket = new \Socket(\Socket::AF_INET, \Socket::SOCK_STREAM, 0); | ||
| $socket->shutdown(); | ||
|
|
||
| var_dump($socket->shutdown(4)); // invalid - false |
There was a problem hiding this comment.
wouldn't an exception be more appropriate here?
| PHP_METHOD(Socket, clearError); | ||
| PHP_METHOD(Socket, strerror); | ||
|
|
||
| #ifdef HAVE_SHUTDOWN |
There was a problem hiding this comment.
where is this macro defined? never mind
classes/socket.h
Outdated
| PHP_METHOD(Socket, shutdown) { | ||
| zend_long how_shutdown = 0; | ||
|
|
||
| if (zend_parse_parameters(ZEND_NUM_ARGS(), "l", &how_shutdown) != SUCCESS) { |
There was a problem hiding this comment.
Is there any reason in particular why we don't default this parameter to 2? I would assume that most of the time a socket is shut down, we'd like to close it for both reads and writes, so following PHP's socket_shutdown function seems like it would make sense.
src/socket.c
Outdated
| } | ||
|
|
||
| #ifdef HAVE_SHUTDOWN | ||
| void pthreads_socket_shutdown(zval *object, zend_bool how_shutdown, zval *return_value) { |
There was a problem hiding this comment.
the signature here is incorrect - how_shutdown should be zend_long not zend_bool.
src/socket.h
Outdated
| void pthreads_socket_send(zval *object, zend_string *buf, zend_long length, zend_long flags, zval *return_value); | ||
| void pthreads_socket_close(zval *object, zval *return_value); | ||
| #ifdef HAVE_SHUTDOWN | ||
| void pthreads_socket_shutdown(zval *object, zend_bool how_shutdown, zval *return_value); |
There was a problem hiding this comment.
same as above (signature is incorrect)
tests/socket-shutdown.phpt
Outdated
| --TEST-- | ||
| Test of Socket::shutdown() - testing params and separate shutdown of read and write channel | ||
| --DESCRIPTION-- | ||
| Test that creating and closing sockets works as expected on all platforms (gh issue #798) |
There was a problem hiding this comment.
this description might need fixing
|
Experiencing test failures: |
tests/socket-shutdown.phpt
Outdated
| $socket = new \Socket(\Socket::AF_INET, \Socket::SOCK_STREAM, 0); | ||
| $socket->shutdown(); | ||
|
|
||
| var_dump($socket->shutdown(4)); // invalid - false |
There was a problem hiding this comment.
On Windows, this causes an ENOTCONN error to be emitted, which causes a RuntimeException to be thrown. I assume that in Winsock2, socket state is checked prior to checking the argument value [citation needed].
There was a problem hiding this comment.
@dktapps I slightly modified the test from bind()/listen() to connect(). Can you check the test on a windows machine?
* Fixed compiler warning - wrong argtypes * Added SHUTDOWN_READ, SHUTDOWN_WRITE, SHUTDOWN_BOTH constants * Utilize new constants in tests * Socket::shutdown() now defaults to shutting down both channels with no parameters * Ensure consistent behaviour when using shutdown() with bad arguments * Fixed Socket::shutdown() arginfo and prototype * Don't return false on invalid argument - we're throwing an exception anyway
3 participants
shutdown()added