[Boost-bugs] [Boost C++ Libraries] #10009: Boost Asio should provide also thread-safe sockets

Subject: [Boost-bugs] [Boost C++ Libraries] #10009: Boost Asio should provide also thread-safe sockets
From: Boost C++ Libraries (noreply_at_[hidden])
Date: 2014-05-06 14:51:27

#10009: Boost Asio should provide also thread-safe sockets
--------------------------------------------+----------------------------
 Reporter:  Vladislav Valtchev <vladi32@…>  |      Owner:  chris_kohlhoff
     Type:  Feature Requests                |     Status:  new
Milestone:  To Be Determined                |  Component:  asio
  Version:  Boost 1.55.0                    |   Severity:  Problem
 Keywords:                                  |
--------------------------------------------+----------------------------
 The feature I'm requesting is strictly related to the well-known problem
 reported in ticket #8538, but I'm not reporting it as a bug since the
 documentation explicitly states that the Asio's sockets are ''not''
 thread-safe ''by design'': the user ''must'' use strands at top level to
 ensure a correct behavior in a multi-threaded environment. If I've
 correctly understood, the rationale for such design is that in a single
 threaded environment, one should pay no cost for synchronization.
 I disagree with this design decision for several reasons.
 == The multi-threaded environments are more important ==
 I believe that the environments where asio is seriously (heavily) used are
 multi-threaded (there is an asio-based thread pool): this means that the
 optimization is made for the simpler cases (where the cost for a not-
 contended lock acquisition is negligible); IMHO this is wrong. This adds
 an additional (significative) overhead to users which have necessarily to:
    1. use strands (through strand::wrap() as suggested in the docs) ''or''
    2. use mutexes (or other lock types) and carry always a reference to
 them with the relative socket ''or''
    3. wrap asio's socket with a custom class that internally uses locking.
 == strand::wrap() is not always that useful ==
 I believe that the solution officially recommended to "solve the problem"
 (wrapping the callbacks with strand::wrap()) is theoretically a bad idea
 since it causes the serialization (which is ''almost'' like what a mutex
 does) of the callbacks invocation: this is ''deeply'' wrong because it is
 an anti-pattern: the fundamental principle of synchronization is that ONLY
 the data has to be "locked", NOT the code. Using strand::wrap() ( ~=
 locking the CODE ) could lead to serious performance bottlenecks if the
 callbacks' code is not ''carefully'' written (one could unconsciously do a
 heavy computation (or sync I/O) through several levels of function calls).
 So, within this model, writing an efficient code will mean to be careful
 and to necessarily re-schedule all non-trivial tasks: that is not so easy
 sometimes.[[br]]
 What if, for example, one wants to close 100 sockets in a callback? Using
 strand::wrap() would mean to use a common strand for all the 100 sockets
 (= to serialize all the callbacks for these sockets): this is an
 ''unacceptable'' cost to pay. Instead, using a strand for each socket and
 rescheduling a close() request using strand::post() is much better but
 would mean always carrying back a reference of the relative strand for
 each socket: this is same problem of the alternative 2: no one would be
 happy to do this.[[br]]
 It seems that the most attractive alternative is the third one.
 == The non-thread-safe sockets are not something one would expect from
 Asio ==
 I'm pretty sure that I'm not the only one who, before reading
 ''carefully'' the documentation, was expecting that the sockets were ''of
 course'' thread-safe: we are dealing with a high-level library for
 asynchronous I/O. The situation is analogue to C++11's atomics: they, by
 default, use FULL barriers because this way is simpler to use them;
 advanced users (that really know what they're doing) instead, could always
 specify the exact type of barrier they need. I think Asio is roughly at
 the same level of abstraction of standard C++ libraries (if not at higher
 one), so I think it should follow the same philosophy.
 == Synchronization is already used in Asio's internals ==
 Even if the official docs declare the asio's sockets as non-thread-safe,
 actually ''there are'' synchronization mechanisms in asio's reactors
 (epoll and kqueue) which allow the sockets to ''almost work'' in a multi-
 threaded environment except when destructive operations like close() are
 used. In these cases, the subtle race described in #8538 appears since the
 mutex protecting the access to the descriptor_data node ''has to be''
 released before "freeing it" because the mutex itself is a member of the
 (same) node. That mutex makes me suspect that ''maybe'' an attempt has
 been made to solve the synchronization problem at the reactor level but it
 failed since the socket's lifetime is bigger than the descriptor_data node
 which has to die when the descriptor is closed. Since the reactors has no
 access to the upper level socket services (such thing will break the
 isolation of the abstraction layers), the only solution seems to use a
 top-level synchronization mechanism.
 == What is my purpose to improve Asio ==
 I purpose a simple patch (the attached file) that adds a lock contended
 only among the async and the destructive operations at the top-level
 classes (the *_socket_service classes). I think this solution is a good
 compromise because all the code it adds is under an #ifdef: who wants to
 work easily with asio's sockets has simply to specify it at compile-time;
 the others could still pay the "zero" cost in single-threaded
 applications. Even if I'd prefer the define to work in the opposite way,
 I'm purposing this change because has a minor impact, conserves the old
 behavior unless specified and so, I hope it has a greater chance to be
 accepted.
 == Just a final remark ==
 I'd just like to say that, even if it probably doesn't emerge from my
 comments, I'm an Asio's fan: overall, it is really a great boost-style
 library and I use it intensively. That's the reason why I'm spending some
 much energies trying to improve it.

-- 
Ticket URL: <https://svn.boost.org/trac/boost/ticket/10009>
Boost C++ Libraries <http://www.boost.org/>
Boost provides free peer-reviewed portable C++ source libraries.