
Show HN: Tacopie, lightweight multi-platform C++11 TCP library - cylix
https://github.com/Cylix/tacopie
======
Matthias247
A little bit of feedback about the overview based on lots of experience with
similar libraries:

\- 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.

~~~
cylix
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 :)

------
DLudwig
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.

~~~
cylix
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!

