Hold XIO server lock across fork about gct HOT 8 CLOSED

Hi @fscheiner, short answer: no it hasn't been merged in. Whether it's still needed I don't know.

from gct.

@msalle

Whether it's still needed I don't know.

Well, if it wasn't changed, I assume it's still a problem, also because of the description in globus/globus-toolkit#63. If the deadlock behaviour can be easily reproduced, we can just compare the results of (1) using the unmodified source and (2) using a "patched" source, to determine if it's needed or not.

from gct.

@msalle
I seem to remember observing globus-gridftp-server processes that keep running w/o producing any load after past transfers were finished. This could be related to the deadlocks described in globus/globus-toolkit#63. So I will try to reproduce this behaviour with the current GCT code locally and then compare to the behaviour with @bbockelm's patch included.

from gct.

The lock is already held during fork().

The fork is done in globus_l_gfs_spawn_child():

And though this function does not lock the mutex, it is called from

I.e. the lock is already held when globus_l_gfs_spawn_child() is called. There is no need to lock it again in this function. Locking the same mutex again from the same thread would be undefined behaviour.

from gct.

Never mind - I now realise that the original PR was for a different mutex than this one. Please disregard my comment.

from gct.

Hi again.

I looked at this as part of preparing my "fix some compiler warnings" PR #179.
I now think that my comment above was not so off the mark after all.
In the source file in question - gridftp/server/src/globus_gridftp_server.c - the mutex globus_l_gfs_xio_server->mutex is never locked or released. All manipulations of globus_l_gfs_xio_server (assignments, function calls, ...) are (or at least are supposed to be) protected by holding the global mutex globus_l_gfs_mutex. So any locking of mutex globus_l_gfs_xio_server->mutex should not be able to persist if the global mutex is released, and hence my comment above is valid.

However, there is a flaw to this logic - which I think is a bug. For some reason, the lock is released here:

inside the globus_l_gfs_spawn_child() function, even tough the logic demands that the mutex should be locked for the duration of this function. The logic demands that the mutex stays locked after returning from the call to globus_l_gfs_spawn_child() and is released by the call to globus_mutex_unlock on line 1270:

As can be seen above, there are some function calls after the return from globus_l_gfs_spawn_child() than uses globus_l_gfs_xio_server, that could potentially screw up the locking on mutex globus_l_gfs_xio_server->mutex without the global lock being locked due to the early release of the lock on line 538.

This unlock also makes no sense for a different reason. Every other call to globus_xio_register_close() is done while the mutex is locked. Here the mutex is released just before calling this function, which makes the call questionable for a different reason.

Dropping the call to release the lock early would fix this.

The direct call to globus_l_gfs_close_cb() in case the callback registration fails must then be handled in some way, since the callback tries to acquire the lock. This could be

        if(result != GLOBUS_SUCCESS)
        {
            globus_mutex_unlock(&globus_l_gfs_mutex);
            globus_l_gfs_close_cb(handle, result, NULL);
            globus_mutex_lock(&globus_l_gfs_mutex);
        }        

But the callback function is:

So this would be equivalent to:

        if(result != GLOBUS_SUCCESS)
        {
            globus_mutex_unlock(&globus_l_gfs_mutex);
            globus_mutex_lock(&globus_l_gfs_mutex);
            globus_i_gfs_connection_closed();
            globus_mutex_unlock(&globus_l_gfs_mutex);
            globus_mutex_lock(&globus_l_gfs_mutex);
        }        

i.e., one could just write:

        if(result != GLOBUS_SUCCESS)
        {
            globus_i_gfs_connection_closed();
        }        

Which would be consistent with the rest of the code in the file, since in other places where, if a callback registration fails in a place where the lock is held, the backup is to execute the code of the callback without the lock/unlock, e.g.:

Coincidently, this change is proposed in the PR #179:
https://github.com/gridcf/gct/pull/179/files#diff-f76f3cae29cf9ff2977b4124b6177a87eb0b1500c72908a7ca6194a6c0e21bd8

from gct.

gct/gridftp/server/src/globus_gridftp_server.c

Line 538 in da14279

globus_mutex_unlock(&globus_l_gfs_mutex);

This came in some 18 years ago. Prior to that, the child was spawned before the locking of globus_l_gfs_mutex had happened (see

But is globus_l_gfs_mutex for the parent the same mutex as globus_l_gfs_xio_server->mutex for the spawned child? I ask, because if not, I don't see a chance for a deadlock in the child. But as @bbockelm detected one in globus/globus-toolkit#63, there must have been one in the code (at least back then).

Won't there be a real chance for a deadlock for the parent in

UPDATE: But maybe not, because if both were the same, the deadlock would - without any changes to line 538 above - already happen in the call to globus_xio_register_open() in

I'll check if the changes from #179 show any differences in behaviour for the Globus GridFTP server compared to the current code state (@da14279). I still wonder how the mentioned deadlocks could happen.

from gct.

I assume this is no longer relevant or needed with the inclusion of #179 (specifically https://github.com/gridcf/gct/pull/179/files#diff-f76f3cae29cf9ff2977b4124b6177a87eb0b1500c72908a7ca6194a6c0e21bd8).

from gct.