mono/mono — pull request #703 by ysw (2013-07-14 21:18:34+00:00)
Title: Fixed: send/recv race in tpool-epoll and tpool-kqueue implementations 

There was a long discussion on a mailing list about the async-socket problem

in mono. We have previously submitted a patch fixing the problem, however it
was later reverted back as causing deadlocks in some cases.

This patch is very close to the original one, but it takes into account how
tpool-thread operates and avoids deadlocks in this code. (Unfortunately, it
probably makes the code a little less readable, but it works).

So, this patch aims to fix several races in the mono code. These problems are
not relevant for a lot of applications as they can appear only if both read
and write operations are active on the socket. (I.e. RPC style communications
do not trigger the problem).

So, there are two major problems with existing mono code:

a) threadpool.c :: socket_io_add

LeaveCriticalSection (&data->io_lock);
data->modify (data->event_data, fd, state->operation, ievt, is_new);

the problem is that data modify receives ievt and is_new arguments which
may be already outdated due to the concurrent operation on the same socket
(read vs. write).

(If you look at the other part of the operation in
tpool_epoll.c :: tp_epoll_wait updates the epoll settigs under the lock.)

b) The epoll implementation uses the following code to change the epoll
settings in the data->modify handler.

if (epoll_ctl (data->epollfd, epoll_op, fd, &evt) == -1) {
int err = errno;
if (epoll_op == EPOLL_CTL_ADD && err == EEXIST) {
epoll_op = EPOLL_CTL_MOD;
if (epoll_ctl (data->epollfd, epoll_op, fd, &evt) == -1) {
g_message ("epoll_ctl(MOD): %d %s", err, g_strerror (err));
}
}
}

Here we have the same problem. If concurrent operation updates epoll between
first epoll_ctl and the second epoll_ctl the second will override concurrent
update.

These problems result in missing callbacks on async socket send or receive
completion

The original idea was as simple as just move the data->modify under the lock.
(NOTE: both internal associated list and epoll are updated under the lock in
case of data->wait (see tp_epoll_wait).

As i already noted it causes a problem for tpool_poll.c implementation as it
uses different approach than epoll/kqueue.

This patch still invokes data->modify under the lock, but allow implementation
to release lock at the point where it is appropriate for the given
implementation.

The tpool-poll.c releases lock immediately and thus its behavior remains
unchanged (and it still has a problem of missing send/receive callbacks)

The tpool-epoll/kqueue implementations update their internal structures and
corresponding OS level structures under the locks (as _wait does). This change
makes them reliable in simultaneous socket read/write scenarios.

The following code demonstrates the problem. See readme inside.

https://github.com/ysw/mono-socket-problem/tree/master/SocketTest

NOTE: you need at least 4 coresto reliably reproduce the problem.

It is quite hard to reproduce on OSX, but it is easy reproducible in LINUX.

This commit is licensed under MIT/X11.