Show HN: Tacopie, lightweight multi-platform C++11 TCP library(github.com)
github.com
Show HN: Tacopie, lightweight multi-platform C++11 TCP library
https://github.com/Cylix/tacopie
8 comments
Wow, thanks a lot for your feedback, I really do appreciate a lot!
for sync/async, that's a good point. I was not sure about how to design it at the beginning and went for simplicity, but having a consistent behavior makes sense. Probably having a default connect that is async as the read/write operations, as well as another sync_connect method would make the library more consistent with the choice of both options.
I have for plan to add more documentation concerning the io service. There is one thread for the event loop, and then IO_SERVICE_NB_WORKERS threads working in a thread pool to process the read and write events polled from the event loop.
Good point for the read buffer, will add the option to specify one
Good point too for the get_clients()!
Thanks again, will have some work to implement these changes, but that's worth it :)
for sync/async, that's a good point. I was not sure about how to design it at the beginning and went for simplicity, but having a consistent behavior makes sense. Probably having a default connect that is async as the read/write operations, as well as another sync_connect method would make the library more consistent with the choice of both options.
I have for plan to add more documentation concerning the io service. There is one thread for the event loop, and then IO_SERVICE_NB_WORKERS threads working in a thread pool to process the read and write events polled from the event loop.
Good point for the read buffer, will add the option to specify one
Good point too for the get_clients()!
Thanks again, will have some work to implement these changes, but that's worth it :)
A suggestion: wrap the bulk of content in platform-specific .cpp files, with pre-processor guards that'll check for platform. This should allow the library to be imported into other projects, simply by importing all .cpp files, and adding a path for header-includes. With that done, adding support for extra build systems becomes an optional, albeit nice-to-have addition, but by no means required.
Hi,
Interesting, haven't thought about that, will probably make that small change in the future on this lib and some others of my libs
Thanks!
Interesting, haven't thought about that, will probably make that small change in the future on this lib and some others of my libs
Thanks!
jkforpres(1)
- The library seems to mix synchronous operations (connect) and asynchronous ones (read/write). Stick to one, everything else doesn't make sense.
- It isn't documented on which thread callbacks are actually executed. It can't be a user-visible eventloop since the user never starts one. So it's probably an eventloop on a background thread. Based on the -DIO_SERVICE_NB_WORKERS=1 variable it might even be multiple background threads. From my experience the majority of users won't expect that and will inevitably run into ugly synchronization problems. Even for those who expect it proper synchronization will be hard to add if operations start and finish on different threads (and sometimes the same threads). I would recommend to stay at a pure singlethreaded eventloop model if asynchronous APIs are the design goal. Even in super-mature libraries like boost asio a mixed use of multithreaded and async operations causes headaches and is far more likely to show implementation bugs.
- Things like read don't accept a buffer as input into which the read should happen but only a size. The buffer is allocated by the library. This is convenient as far as it doesn't force the caller to guarantee that the buffer outlives the operation. But it will cause a lot of memory churn, which is in my experience of the most typical reasons of performance degradation.
- const std::list<std::shared_ptr<tacopie::tcp_client> >& tacopie::tcp_server::get_clients() const : Returning a reference to a non-synchronized list in a multithreaded environment seems inevitably broken.