Re: [Boost-users] Concurrency and session termination in boost::asio

Subject: Re: [Boost-users] Concurrency and session termination in boost::asio
From: Scott Gifford (sgifford_at_[hidden])
Date: 2008-11-06 01:36:24

Boris <boriss_at_[hidden]> writes:

> On Wed, 05 Nov 2008 20:39:47 +0100, Scott Gifford
> <sgifford_at_[hidden]> wrote:
>
>> [...]I rewrote it to have write errors handled by closing the session,
>> which causes the read callback to run, notice the session has been
>> closed, and delete the object.  This seemed to solve the problem, but
>> it still seems there is a potential problem if the read callback
>> finishes deleting the object while the write callback is running.
>>
>> Is there a better way to handle session termination?
>
> See the various Boost.Asio examples where classes with asynchronous
> handlers are typically derived from boost::enable_shared_from_this.

Ah, thanks!  I had noticed they were using shared_ptr<>s, but not that
they were passing them to boost::bind, very clever.

So I have changed my server's session class to inherit from
enable_shared_from_this and changed the calls to boost::bind to use
the shared this pointer.  Now as long as there is a registered
callback for a session object, it will never be destroyed.  If I don't
hold any external references to the session object, when all the
callbacks complete without starting any other asynchronous calls, the
object will be destroyed after last callback completes, in the
destructor for the shared_ptr.

If the socket fails or is closed, all of the callbacks will be called
with a failure code, ensuring that the session is eventually
destroyed.

Since my server is always reading from clients, I can just make sure
there is always a registered callback for network reads.  I can do
this by starting an asynchronous read when the session object is
created, then making sure that when each read completes I start a new
one.  That will ensure that a reference to my session stays alive, and
to shut down the connection the read callback will simply not start
another asynchronous read.

Does all of that sound about right?
 
One related question: In my server, on certain errors or a client
request, I was calling the close() method of a tcp::socket.  Watching
that in the debugger, it ends up running:

    boost::asio::detail::reactive_socket_service::close

from

    boost/asio/detail/reactive_socket_service.hpp

The close method calls "is_open" to determine if the socket has
already been closed, and if it is not asks the OS to close it.

It looks to me like this is not done in a threadsafe way.  I can't see
a mutex being held here, and so two threads that call close()
simultaneously could both find that is_open returned false, then both
close the socket.  With unfortunate timing, by the time the second one
runs, that socket's file descriptor could have been reused, and the
wrong fd would be closed.

Am I missing something here?  Or is close() not meant to be
threadsafe?

Thanks!

----Scott.