fpagliughi / sockpp

Modern C++ socket library.
BSD 3-Clause "New" or "Revised" License
792 stars 126 forks source link

Fixes and Additions #81

Closed Ace-Krypton closed 1 year ago

Ace-Krypton commented 1 year ago

Added a new poll()-based implementation for the connector::connect() method, replacing the select()-based implementation for systems with many sockets.

Implemented error handling for the case where bind() fails in the datagram_socket::datagram_socket() constructor, by closing the socket and returning false.

Implemented error handling for the case where recvfrom() returns a negative value in the datagram_socket::recv_from() method, by returning zero.

Fixed the return type of the datagram_socket::recv_from() method from ssize_t to int for consistency with the other methods in the class.

Fixed potential buffer overflows

fpagliughi commented 1 year ago

Hey, thanks for the PR. Actually swapping out select() for poll() was on my short list of things to do, so this will save me the trouble.

What platforms did you test this against?

There's some new unstable work in the develop branch to implement #77, to start using std::error_code and remove exceptions, so I will need see that this will merge and then update for that new direction.

Ace-Krypton commented 1 year ago

tbh, I haven't done any extensive tests on it yet, I was a bit busy. I just did the //TODO comments, and tried to remove some obvious buffer overflows. You might wanna give it a run.

Ace-Krypton commented 1 year ago

About error handling, I will try to modify the code to implement std::error_code. Didn't noticed that

fpagliughi commented 1 year ago

I’m mainly wondering if the standard poll() works on Windows. I vaguely remembering there being a special version for WinSock, but that was a long time ago. Don’t know if it’s changed now. I’m not much of a Windows programmer.

Ace-Krypton commented 1 year ago

I think poll() will work fine in most windows versions, but I haven't been using windows for over 6 years, I don't have much experience. After searching a bit, I found that there is poll() equivalent which is WSAPoll() function in windows. I don't have windows machine, so I didn't have the chance to try it.

fpagliughi commented 1 year ago

OK. I can test it.

tan-wei commented 1 year ago

The PR is closed. So I wonder that if poll()-based implementation is in the plan. Thanks.

fpagliughi commented 1 year ago

Yes. At least for non-Windows systems. In the short term, it looks like a quick win is to leave select() on Windows and use poll() on other (*nix/posix) systems. There's already a solution up in the develop branch:

https://github.com/fpagliughi/sockpp/blob/f81589e824c0d59b52e5db5ee2c0fd0329a5b99d/src/connector.cpp#L80-L141

After that I will start looking into the WSAPoll() function on Windows, to see if that would work. But it's a fairly low priority for me at the moment.