Fixed: send/recv race in tpool-epoll and tpool-kqueue implementations wi...

Hi,

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.

thank you
Yuriy