fpagliughi / sockpp

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

Non-blocking I/O #25

Closed snej closed 4 years ago

snej commented 4 years ago

It would be useful to support non-blocking I/O on sockets, and non-blocking connect and accept. This allows a lot more flexibility with threading, instead of having to devote one or two threads to every socket. It also seems like the only way to control the timeout of connect.

I've implemented this and will try to put a PR together today.

fpagliughi commented 4 years ago

Yeah, funny... I have this and scatter/gather (#26) working for Linux, but I haven't pushed yet! I was just reading on how to do it for Windows, and it looks workable to do a in a way that's platform agnostic.

I put it in the base socket class:

bool set_non_blocking(bool on=true);

But I actually only tested it for reading and writing. I didn't think of testing accept().

I was trying to think of a couple things that were relatively universal but implemented differently on the different targets. This was the first thing that came to mind.

If you're far along, PR what you have and I will try to merge them.

snej commented 4 years ago

Yes, we have it working, with Windows support by @borrrden.

I named the call set_blocking(bool blocking) to avoid double negatives, but maybe your approach is better since everyone thinks of the feature as "non-blocking" and might get confused...

fpagliughi commented 4 years ago

Yeah. I think I would definitely want to keep it as set_non_blocking(bool). That's the convention.

I'm trying to not stray too far from the basic C API. Warts and all.

fpagliughi commented 4 years ago

BTW, I think there are enough little fixes and features to push out a v0.7 release, but then I need to get to a couple of, umm more popular libraries that a lot of people are waiting on, before figuring out what to do about SSL/TLS.

borrrden commented 4 years ago

If you have a set of tests you'd like to be run on Windows then I wouldn't mind running them a few times so that you can be somewhat satisfied that things work well there too.

fpagliughi commented 4 years ago

I spent all of 10 minutes trying to get Catch2 working on Windows to run the unit tests. It didn't compile immediately, and I moved on. But it would be good to get it set up to run the unit tests on Windows. Even better to set up Appveyor on GitHub.

fpagliughi commented 4 years ago

I added my implementation to the 'develop' branch. The Windows version is pretty straight forward, but I haven't had a chance to test it yet.

borrrden commented 4 years ago

I was able to make it compile and run. The hardest part, actually, was fighting with CMake since Catch2 is not something I normally "install" since it's just one (or two) headers. Some CMake patching took care of that though, and the only thing left is the following issues (on Windows) in the unit tests causing 7 test and 8 assertion failures:

All of the above are pretty easily fixable though.

EDIT OH I forgot, initialize() is never called during the tests so a lot of tests initially failed with the "not initialized" error. I fixed that by not auto generating the catch main function (GOTCHA: Include socket platform.h before catch.hpp to avoid odd errors from including headers in the wrong order)

fpagliughi commented 4 years ago

@borrrden If you have some hints on the required CMake changes, and/or what you did to satisfy the "install" issue, let me know, Then I can go through and fix the other issues in the tests themselves.

borrrden commented 4 years ago

It depends on what your intended strategy is for obtaining the catch header. We also use catch and we simply include the header in the repo since it is easily portable but the changes I made I wouldn't consider acceptable for a widespread use case, they were just hacks so that I could move forward. If you want to continue using find_package then I'll try to figure out where Windows wants it in that case. Basically all you need, though, is to make sure that catch2/catch.hpp is in your include search path.

fpagliughi commented 4 years ago

We went a little off-topic here. I created a new issue for getting the unit tests passing on more than just Linux: #33

fpagliughi commented 4 years ago

Non-blocking I/O seems to be working on Windows and Linux.