Update sockets module #301

Javivi · 2015-10-11T22:37:10Z

Been working on this some time. Changelog:

WinSock version changed from 1.1 to 2.2.
Properly check for WinSock initialization on OnAmxxAttach/Detach.
Now natives will not be added if we can't start up WinSock.
socket_open() is now IP version agnostic (both IPv4 and IPv6 are
supported).
Error reporting has been changed on socket_open(), a new parameter
called _libc_errors has been added, and, if enabled, libc errors will be
returned instead of the previous made-up errors.
socket_close() now returns a value on success/failure.
Added non-blocking sockets at socket_open_nb().
Added socket_is_writable() to check if a socket is ready for write.
Added socket_is_readable() as an alias to socket_change().
Code rewritten to be more readable, it should be self-explaining now.

Sorry for the ugly diff @Arkshine

- WinSock version changed from 1.1 to 2.2. - Properly check for WinSock initialization on OnAmxxAttach/Detach. - Now natives will not be added if we can't start up WinSock. - socket_open() is now IP version agnostic (both IPv4 and IPv6 are supported). - Error reporting has been changed on socket_open(), a new parameter called _libc_errors has been added, and, if enabled, libc errors will be returned instead of the previous made-up errors. - socket_close() now returns a value on success/failure. - Added non-blocking sockets at socket_open_nb(). - Added socket_is_writable() to check if a socket is ready for write. - Added socket_is_readable() as an alias to socket_change(). - Code rewritten to be more readable, it should be self-explaining now.

Updated documentation following the guidelines

Arkshine · 2016-01-17T10:37:11Z

modules/sockets/sockets.cpp

+	FD_ZERO(&readfds);
+	FD_SET(sockfd, &readfds);
+
+	return (select(sockfd + 1, &readfds, nullptr, nullptr, &tv) > 0) ? 1 : -1;


You will break likely compatibility by returning -1 instead of 0 originally.

It was changed from 0 to -1 to maintain consistency with the other function's return value, because having some functions returning 0 and others returning -1 could be confusing.
Also I didn't thought anyone would check the socket against 0, I thought they would just do !socket.

Return values should be standardized across the module I think, so maybe changing all of them to 0 would be better, even if some of them return -1 by design.

Just doing if (socket_change()) will still break with -1 since expression will be considerated as boolean and because it's a non-zero value, condition will be true.

So yes it should be standardized as much as possible, but unfortunately you can't change suddenly 0 -> -1 or -1 -> 0.

* Receives data. * * @note The ammount of bytes than you end up receiving can be less than the one you expected * * @param _socket Socket descriptor * @param _data Array to save the data * @param _length Length of the array * * @return Ammount of bytes received * 0 if the peer closed the connection * -1 on failure */ native socket_recv(_socket, _data[], _length);

Every return value but this one can be changed to 0 without any problem.

Changing socket_recv() to 0 would remove the possibility of knowing if the peer closed the connection.

Should we remove this feature in order to keep everything consistent ? I'm not sure it would be of any use, and the socket's status can be check using other functions.

You don't want to break backward compatibility, so no you can't change return value here.

I just took a look again at it[1] and yeah, it has to stay at -1. It could return 0 previously, I just documented it.

[1] https://github.com/alliedmodders/amxmodx/blob/master/modules/sockets/sockets.cpp#L143

- Fixed the backwards compatibility with the return codes - Merged socket_connect and socket_connect_nb - Added a 5th parameter to socket_open that takes bit flags to enable the new features (libc errors & nonblocking sockets) - Fixed an error on socket_send2 that caused the buffet not to start from the beginning if multiple calls were made - Updated docs - [docs] Prefixed error codes with SOCK_ - [docs] Added the new flags SOCK_NON_BLOCKING and SOCK_LIBC_ERRORS - [docs] Added a new stock called SOCK_ERROR_EINPROGRESS(error) to be used when checking if a newly created nonblocking socket is connecting

Javivi · 2016-04-02T20:12:44Z

Simple test plugin

#include < amxmodx >
#include <sockets>

#define NODE "httpbin.org"
#define SERVICE 80

new request[] = "GET /get HTTP/1.1^nHOST: httpbin.org^nConnection: close^n^n^n^n"

public plugin_init( )
{
    register_plugin("Socket test", "1.0", "Javivi")

    register_clcmd("say /s1", "test_blocking")
    register_clcmd("say /s2", "test_nonblocking")
}

public test_blocking(id)
{
    new sockfd, socket_error
    new bytes_sent, bytes_recv, bytes_recv_buffer[500]

    sockfd = socket_open(NODE, SERVICE, SOCKET_TCP, socket_error, SOCK_LIBC_ERRORS)

    if(sockfd != -1)
    {
        client_print(id, print_chat, "Got socketfd %d", sockfd)

        bytes_sent = socket_send(sockfd, request, charsmax(request))
        client_print(id, print_chat, "[%d/%d] bytes sent", bytes_sent, charsmax(request))

        bytes_recv = socket_recv(sockfd, bytes_recv_buffer, charsmax(bytes_recv_buffer))
        client_print(id, print_chat, "[%d/%d] bytes received", bytes_recv, charsmax(bytes_recv_buffer))
        client_print(id, print_chat, "%s", bytes_recv_buffer)

        socket_close(sockfd)
    }
    else
    {
        client_print(id, print_chat, "Error %d", socket_error)
    }
}

new nb_sockfd

public test_nonblocking(id)
{
    new socket_error

    nb_sockfd = socket_open(NODE, SERVICE, SOCKET_TCP, socket_error, SOCK_NON_BLOCKING|SOCK_LIBC_ERRORS)

    if(nb_sockfd != -1 && SOCK_ERROR_EINPROGRESS(socket_error))
    {
        client_print(id, print_chat, "Got nonblocking socket %d", nb_sockfd)

        set_task(1.0, "check_nbsocket", 1234, .flags="b")
    }
    else
    {
        client_print(id, print_chat, "Error %d", socket_error)
    }
}

public check_nbsocket(task)
{
    if(socket_is_writable(nb_sockfd))
    {
        remove_task(task)

        client_print(0, print_chat, "Socket %d is writable", nb_sockfd)

        new bytes_sent = socket_send(nb_sockfd, request, charsmax(request))
        client_print(0, print_chat, "[%d/%d] bytes sent", bytes_sent, charsmax(request))

        set_task(1.0, "recv_nbsocket", 1234, .flags = "b")
    }
}

public recv_nbsocket(task)
{
    new bytes_recv, bytes_recv_buffer[500]

    bytes_recv = socket_recv(nb_sockfd, bytes_recv_buffer, charsmax(bytes_recv_buffer))

    if(bytes_recv)
    {
        remove_task(task)

        client_print(0, print_chat, "[%d/%d] bytes received", bytes_recv, charsmax(bytes_recv_buffer))
        client_print(0, print_chat, "%s", bytes_recv_buffer)

        socket_close(nb_sockfd)
    }
}

NOTE: The first call to /s2 will probably block, as the name resolution server will have to be called. To avoid that a numeric ip address has to be used, as noted in the docs.

FOR THE FUTURE:

Add a new function that lets you set more flags on the socket (like TCP_CORK, TCP_NODELAY...)
Add SSL support
Add forwards (can be done easily but there's problems with it being multiplatform)

WPMGPRoSToTeMa · 2016-04-03T09:52:42Z

@Javivi ^n -> ^r^n?
You can resolve name in another thread. (non-blocking socket_open)

Javivi · 2016-04-03T12:17:23Z

Both ways adhere to the HTTP requisite of a blank line at the end, but yes, if the test plugin doesn't work for someone try changing ^n^n for ^r^n.

Do you mean creating a new function that spawns a thread and then callbacks when the socket is "ready" ?

We discussed at the beginning of the pull against having 2 separate functions for connect when just 1 would do, and also keep in mind that once getaddrinfo resolves an address it stays on the dns cache until it gets flushed, and that may never happen.

Using a numeric IP or having the dns address already cached makes the socket_open function with the NON_BLOCKING flag not to block, adding a new function with threads just for doing a resolve adds some unneeded complexity in my opinion.

WPMGPRoSToTeMa · 2016-04-04T23:59:56Z

@Javivi hm, you can add a separate function for non-blocking domain resolving.

Javivi · 2016-04-05T07:02:58Z

You mean doing something like:
1 - socket_dnspreload("domain.com")
2 - spawn thread and call getaddrinfo
3 - callback

That can be done, but I don't think it is needed. You only need to resolve the hostname one time and it stays cached until the dns cache gets flushed.

Calling socket_open just one time at plugin_init or at any event that doesn't interfere with a current game would do the same, even though it is not as pretty

Unless the dns of the domain is really misconfigured the query should not take more than a few ms and it can even return instantly, I would like some feedback on this if possible too https://github.com/javivi/amxmodx/tree/sockets-update

Having an event based module is obviously the end game, and there won't be any problems with this when that happens, but meanwhile a work around is needed.

Arkshine · 2017-02-18T22:47:46Z

The changes look overall okay.

Code could have modernized a little more with C++11/AMTL but it's okay as it is.
I'm not totally sure about socket_is_readable as native. If just an alias of socket_change, a stock would be enough I think, but it's more consistent and readable this way and if following this way, you should probably deprecate socket_change similar to https://github.com/alliedmodders/sourcemod/blob/master/plugins/include/string.inc#L92-L100.
Documentation needs likely some adjustments. Comment on constants should use /** */ (i.e: https://github.com/alliedmodders/amxmodx/blob/master/plugins/include/amxconst.inc). English is not my forte. 'Check should be Checks. You should probably cut the lines (~80/120 characters?). Klippy did some update in another PR https://github.com/rsKliPPy/amxmodx/blob/2165ffb4b5ef84cc84e03744e41b831627a272bf/plugins/include/sockets.inc, you could probably take some inspiration from there and mixing things.

Are there others things you wanted to add?

Fix some typos, shorten lines, document SOCK_ERROR_EINPROGRESS

No more magic

This reverts commit 0f23329.

It should not be needed because nb sockets should always be checked or writability

strncopy will stop on a null byte, that makes the function unusable to receive binary data

Javivi · 2017-02-22T23:59:29Z

Should be ready for merge if nothing wrong pops.

Spaguetti test code.

/s1 uses socket_open() + socket_send()
/s2 uses socket_open() with the new nonblocking and libc errors flags + socket_send() + socket_is_writable(). Messing around with the NODE/SERVICE on socket_open() will show some of the new libc errors
/s3 uses socket_open() + socket_send2() with a null byte
/s4 uses socket_open() + socket_send() with a null byte

/s1 and /s2 should print something like this on console

HTTP/1.1 200 OK
Server: nginx
Date: Wed, 22 Feb 2017 23:47:01 GMT
Content-Type: application/json
Content-Length: 155
Connection: close
Access-Control-Allow-Origin: *
Access-Control-Allow-Credentials: true

{
"args": {},
"headers": {
"Ho
st": "httpbin.org",
"Test": "a\u00ffc"
},
"origin": "1.2.3.4",
"url": "http://httpbin.org/get"
}

/s3 should print

HTTP/1.1 400 Bad Request
Server: nginx
Date: Wed, 22 Feb 2017 23:54:25 GMT
Content-Type: text/html
Content-Length: 166
Connection: close
<title>400 Bad Request</title>
400 Bad Request

nginx

/s4 should crash the server

#include <amxmodx>
#include <sockets>

#define NODE "httpbin.org"
#define SERVICE 80

public plugin_init( )
{
    register_clcmd("say /s1", "test_blocking")
    register_clcmd("say /s2", "test_nonblocking")
    register_clcmd("say /s3", "test_send2")
    register_clcmd("say /s4", "test_send12")
}

public test_blocking(id)
{
    new sockfd, socket_error
    new bytes_sent, bytes_recv, bytes_recv_buffer[500], bytes_recv_buffer_printed

    sockfd = socket_open(NODE, SERVICE, SOCKET_TCP, socket_error, SOCK_LIBC_ERRORS)

    if(sockfd != -1)
    {
        client_print(id, print_chat, "Got socketfd %d", sockfd)

        new request[100]
        formatex(request, charsmax(request), "GET /get HTTP/1.1^nHOST: httpbin.org^nTEST: %c%c%c^nConnection: close^n^n^n^n", 'a', -1, 'c')
        
        bytes_sent = socket_send(sockfd, request, strlen(request))
        client_print(id, print_chat, "[%d/%d] bytes sent", bytes_sent, charsmax(request))

        bytes_recv = socket_recv(sockfd, bytes_recv_buffer, charsmax(bytes_recv_buffer))
        client_print(id, print_chat, "[%d/%d] bytes received", bytes_recv, charsmax(bytes_recv_buffer))

        bytes_recv_buffer_printed = server_print("%s", bytes_recv_buffer)
        server_print("%s", bytes_recv_buffer[bytes_recv_buffer_printed-1])

        socket_close(sockfd)
    }
    else
    {
        client_print(id, print_chat, "Error %d", socket_error)
    }
}

new nb_sockfd

public test_nonblocking(id)
{
    new socket_error

    nb_sockfd = socket_open(NODE, SERVICE, SOCKET_TCP, socket_error, SOCK_NON_BLOCKING|SOCK_LIBC_ERRORS)

    if(nb_sockfd != -1)
    {
        client_print(id, print_chat, "Got nonblocking socketfd %d", nb_sockfd)

        set_task(1.0, "check_nbsocket", 1234, .flags="b")
    }
    else
    {
        client_print(id, print_chat, "Error %d", socket_error)
    }
}

public check_nbsocket(task)
{
    if(socket_is_writable(nb_sockfd))
    {
        remove_task(task)

        client_print(0, print_chat, "Socket %d is writable", nb_sockfd)

        new request[100]
        formatex(request, charsmax(request), "GET /get HTTP/1.1^nHOST: httpbin.org^nTEST: %c%c%c^nConnection: close^n^n^n^n", 'a', 'b', 'c')

        new bytes_sent = socket_send(nb_sockfd, request, strlen(request))
        client_print(0, print_chat, "[%d/%d] bytes sent", bytes_sent, charsmax(request))

        set_task(1.0, "recv_nbsocket", 1234, .flags = "b")
    }
}

public recv_nbsocket(task)
{
    new bytes_recv, bytes_recv_buffer[500], bytes_recv_buffer_printed

    bytes_recv = socket_recv(nb_sockfd, bytes_recv_buffer, charsmax(bytes_recv_buffer))

    if(bytes_recv)
    {
        remove_task(task)

        client_print(0, print_chat, "[%d/%d] bytes received", bytes_recv, charsmax(bytes_recv_buffer))

        bytes_recv_buffer_printed = server_print("%s", bytes_recv_buffer)
        server_print("%s", bytes_recv_buffer[bytes_recv_buffer_printed-1])

        socket_close(nb_sockfd)
    }
}

public test_send2(id)
{
    new sockfd, socket_error
    new bytes_sent, bytes_recv, bytes_recv_buffer[500], bytes_recv_buffer_printed

    sockfd = socket_open(NODE, SERVICE, SOCKET_TCP, socket_error, SOCK_LIBC_ERRORS)

    if(sockfd != -1)
    {
        client_print(id, print_chat, "Got socketfd %d", sockfd)

        new request[100]
        formatex(request, charsmax(request), "GET /get HTTP/1.1^nHOST: httpbin.org^nTEST: %c%c%c^nConnection: close^n^n^n^n", 'a', 0, 'c')
        
        bytes_sent = socket_send2(sockfd, request, charsmax(request))
        client_print(id, print_chat, "[%d/%d] bytes sent", bytes_sent, charsmax(request))

        bytes_recv = socket_recv(sockfd, bytes_recv_buffer, charsmax(bytes_recv_buffer))
        client_print(id, print_chat, "[%d/%d] bytes received", bytes_recv, charsmax(bytes_recv_buffer))

        bytes_recv_buffer_printed = server_print("%s", bytes_recv_buffer)
        server_print("%s", bytes_recv_buffer[bytes_recv_buffer_printed-1])

        socket_close(sockfd)
    }
    else
    {
        client_print(id, print_chat, "Error %d", socket_error)
    }
}

public test_send12(id)
{
    new sockfd, socket_error
    new bytes_sent, bytes_recv, bytes_recv_buffer[500], bytes_recv_buffer_printed

    sockfd = socket_open(NODE, SERVICE, SOCKET_TCP, socket_error, SOCK_LIBC_ERRORS)

    if(sockfd != -1)
    {
        client_print(id, print_chat, "Got socketfd %d", sockfd)

        new request[100]
        formatex(request, charsmax(request), "GET /get HTTP/1.1^nHOST: httpbin.org^nTEST: %c%c%c^nConnection: close^n^n^n^n", 'a', 0, 'c')
        
        bytes_sent = socket_send(sockfd, request, charsmax(request))
        client_print(id, print_chat, "[%d/%d] bytes sent", bytes_sent, charsmax(request))

        bytes_recv = socket_recv(sockfd, bytes_recv_buffer, charsmax(bytes_recv_buffer))
        client_print(id, print_chat, "[%d/%d] bytes received", bytes_recv, charsmax(bytes_recv_buffer))

        bytes_recv_buffer_printed = server_print("%s", bytes_recv_buffer)
        server_print("%s", bytes_recv_buffer[bytes_recv_buffer_printed-1])

        socket_close(sockfd)
    }
    else
    {
        client_print(id, print_chat, "Error %d", socket_error)
    }
}

Arkshine

Thanks for testing again and fixing the left issues as we discussed.
It should be enough good now for a first update.

Others features can be done in another PR.
Let's do it then!

Javivi added 2 commits October 11, 2015 23:20

Update docs and fix AMBuild

9674e13

Updated documentation following the guidelines

Arkshine added the enhancement label Nov 22, 2015

Arkshine reviewed Jan 17, 2016
View reviewed changes

Arkshine added the module: socket label Feb 18, 2017

Javivi added 11 commits February 20, 2017 17:24

Fixes for the docs

386ce17

Fix some typos, shorten lines, document SOCK_ERROR_EINPROGRESS

Document magic numbers

0f23329

No more magic

Revert "Document magic numbers"

65adf18

This reverts commit 0f23329.

More docs fixes

a5ef2db

Fix broken socket_send2()

0af5dc7

Add error checking in send2()

053d887

Remove stock

26c6bb3

It should not be needed because nb sockets should always be checked or writability

Fix some identations

c1fd0fc

Fix return codes doc

aa7255d

Fix socket_recv() regression

d69430e

strncopy will stop on a null byte, that makes the function unusable to receive binary data

More docs typo fixes

8cc2ab7

Arkshine approved these changes Mar 3, 2017

View reviewed changes

Arkshine merged commit 6a55312 into alliedmodders:master Mar 3, 2017

Amaroq7 mentioned this pull request Mar 4, 2017

Fix sockets crash #418

Merged

Javivi deleted the sockets-update branch January 27, 2018 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update sockets module #301

Update sockets module #301

Javivi commented Oct 11, 2015

Arkshine Jan 17, 2016

Javivi Jan 17, 2016

Arkshine Jan 18, 2016

Javivi Jan 18, 2016

Arkshine Jan 18, 2016

Javivi Jan 18, 2016

Javivi commented Apr 2, 2016

WPMGPRoSToTeMa commented Apr 3, 2016

Javivi commented Apr 3, 2016

WPMGPRoSToTeMa commented Apr 4, 2016

Javivi commented Apr 5, 2016

Arkshine commented Feb 18, 2017

Javivi commented Feb 22, 2017

400 Bad Request

Arkshine left a comment

Update sockets module #301

Update sockets module #301

Conversation

Javivi commented Oct 11, 2015

Arkshine Jan 17, 2016

Choose a reason for hiding this comment

Javivi Jan 17, 2016

Choose a reason for hiding this comment

Arkshine Jan 18, 2016

Choose a reason for hiding this comment

Javivi Jan 18, 2016

Choose a reason for hiding this comment

Arkshine Jan 18, 2016

Choose a reason for hiding this comment

Javivi Jan 18, 2016

Choose a reason for hiding this comment

Javivi commented Apr 2, 2016

WPMGPRoSToTeMa commented Apr 3, 2016

Javivi commented Apr 3, 2016

WPMGPRoSToTeMa commented Apr 4, 2016

Javivi commented Apr 5, 2016

Arkshine commented Feb 18, 2017

Javivi commented Feb 22, 2017

400 Bad Request

Arkshine left a comment

Choose a reason for hiding this comment