Skip to content
This repository has been archived by the owner on Dec 16, 2019. It is now read-only.

WIP: #776 - socket_recvfrom / socket_sendto support #780

Merged
merged 11 commits into from
Jan 29, 2018

Conversation

sirsnyder
Copy link
Collaborator

Support of socket_recvfrom() and socket_sendto(), equal to php socket API

src/socket.c Outdated
recv_buf = zend_string_alloc(len + 1, 0);

switch (threaded->store.sock->type) {
case AF_UNIX:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will need a check for win32

src/socket.c Outdated
int retval;

switch (threaded->store.sock->type) {
case AF_UNIX:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@sirsnyder sirsnyder force-pushed the feature/776_socket_recvfrom_sendto branch from a18b96e to 88eec0c Compare December 5, 2017 13:24
@sirsnyder
Copy link
Collaborator Author

check for win32 added, @dktapps

@dktapps
Copy link
Contributor

dktapps commented Jan 12, 2018

I haven't had chance to test this yet, will take a look over the weekend.

@dktapps
Copy link
Contributor

dktapps commented Jan 13, 2018

Some issues I picked up on:

  • The extension can't be compiled without PHP sockets. Use of PHP_SOCKET_ERROR() makes pthreads dependent on the sockets extension, as does usage of the below functions. Shouldn't that be PTHREADS_SOCKET_ERROR()?
socket.obj : error LNK2019: unresolved external symbol PHP_SOCKET_ERROR referenced in function pthreads_socket_recvfrom
socket.obj : error LNK2019: unresolved external symbol php_set_inet_addr referenced in function pthreads_socket_sendto
socket.obj : error LNK2019: unresolved external symbol php_set_inet6_addr referenced in function pthreads_socket_sendto
  • MSVC complains about a bunch of unreferenced variable declarations that should probably be moved under their relevant cases.
ext\pthreads\src\socket.c(827): warning C4101: 'addr6': unreferenced local variable
  • sockaddr_un is not defined on Windows, again the declarations could probably be moved under the AF_UNIX cases.

@dktapps
Copy link
Contributor

dktapps commented Jan 13, 2018

@sirsnyder I've created a few commits you might be interested in cherry-picking (commits 1a1f562, 60401fe and d644417).

I've tested this and it seems to be working fine with my additional patches, but one problem I did spot:

socket_recvfrom($socket, $buf, 65535, 0, $addr, $port);

works, and I don't have to define $buf, $addr and $port separately (since they are set by socket_recvfrom() by reference), but

$socket->recvfrom($buf, 65535, 0, $addr, $port);

raises TypeErrors (Fatal error: Uncaught TypeError: Argument 1 passed to Socket::recvfrom() must be of the type string, null given), requiring those variables to be separately declared. Maybe consider allowing the reference parameters to be nullable.

@dktapps
Copy link
Contributor

dktapps commented Jan 13, 2018

Also, I haven't tested IPv6 or Unix socket yet.

@dktapps
Copy link
Contributor

dktapps commented Jan 26, 2018

@sirsnyder please also take a look at pmmp/ext-pmmpthread@27ee665 and pmmp/ext-pmmpthread@2300bcc

src/socket.c Outdated
sin6.sin6_family = AF_INET6;

if (port == NULL) {
efree(recv_buf);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be zend_string_free()? (it is also the same in PHP sockets though)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right. This should be ```zend_string_free()``, changed.

sirsnyder and others added 4 commits January 28, 2018 23:58
it took me about half an hour to find this...
this matches the behaviour of PHP sockets, not requiring that the variables be pre-initialized to the correct type.
@sirsnyder
Copy link
Collaborator Author

Thanks for your commits @dktapps! Would you report the efree/zend_string_free issue to php/php-src? Otherwise I would do that.

@dktapps
Copy link
Contributor

dktapps commented Jan 29, 2018

@sirsnyder reported, thanks.

@sirsnyder sirsnyder merged commit f3c1be5 into krakjoe:master Jan 29, 2018
@sirsnyder sirsnyder deleted the feature/776_socket_recvfrom_sendto branch January 29, 2018 12:29
--SKIPIF--
<?php
if (substr(PHP_OS, 0, 3) == 'WIN') {
die('skip Not valid for Windows');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is valid if compiled with --enable-ipv6. The AppVeyor configure options might need some adjustments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you adjust AppVeyor to support IPV6?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll create a PR. Also, some of these tests aren't working correctly (see https://travis-ci.org/krakjoe/pthreads/jobs/334677874#L800)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a travis issue travis-ci/travis-ci#8711

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the time being fixed with eaa0a58

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants