Removed LS locking for processing incoming connection

[ethouris]

This change frees the call to processing connection from locking on LSLock. This mutex was only intended to guard the fact that particular socket is a listener set up to particular port and nothing else. So handshake processing may continue on this socket, even if in the meantime this socket was removed from being listener. The processing may fail due to this, but there's no reason to keep the listener non-replaceable during this process.

[maxsharabayko]

Is the listener (m_listener) removal or creation protected by some other lock, so it is not deleted while in the CRcvQueue::worker_ProcessConnectionRequest(..)?

[ethouris]

Depends on what you are asking actually. If you are asking about the object assigned there, this is just a normal socket that the user is creating. Activities around assigning its pointer to m_pListener result from either the srt_listen() calls or closing a socket, and these are protected by the m_LSLock mutex.

[maxsharabayko]

I am asking if it is safe to use the listener pointer (assigned as CUDT* listener = m_pListener; under m_LSLock protection) without the m_LSLock protection. Can a socket be closed from another thread while the listener is in use?

[ethouris]

That's exactly the thing I'm not sure how to solve. As I review the code, it might be unfortunately possible. Might also be that the lock on m_LSLock has prevented the closing activity from continuing because a listener socket must be un-listened first before closing and this is stopped here. Note of course that closing a socket doesn't mean that it is being deleted - it's moved to closedsockets container and it stays there for the next 1 second. It is however dangerous provided that we don't know at what moment the socket deletion would be undertaken.

There are some possibilities how to solve it. As the most sensibly sounding would be to lock the m_ControlLock on the listener socket for the time of the call of this function. Another alternative is to lock m_ConnectionLock, but this is controversial already by itself, and I'm not even sure if this really is in use by listener sockets. The first one at least will prevent the simultaneous call to srt_close from moving past the first instruction.

[maxsharabayko]

int srt::CRcvQueue::setListener(CUDT* u)
{
    ScopedLock lslock(m_LSLock);

    if (NULL != m_pListener)
        return -1;

    m_pListener = u;
    return 0;
}

void srt::CRcvQueue::removeListener(const CUDT* u)
{
    ScopedLock lslock(m_LSLock);

    if (u == m_pListener)
        m_pListener = NULL;
}

[ethouris]

This is unrelated. Our dilemma currently is how to make sure that the socket isn't going to be deleted during the processing of spawning a new socket from the listener. This what you cited is only the activity to pin the listener into the multiplexer (queues are underlying objects of the multiplexer).

Also, once the listener is set in the multiplexer, it can only be removed by closing the listener socket (a socket cannot be un-listened any other way).

[maxsharabayko]

CRcvQueue::removeListener(..) is called once the listener is to be closed. It requires holding the m_LSLock to set the m_pListener to NULL. Therefore it indirectly protect the listener to be closed with the CRcvQueue::worker_ProcessConnectionRequest(..) works with it.

[ethouris]

Yes, that's one method of protecting it, but this method gets in conflict with a potential lock inversion. That's what this PR is all about.

[maxsharabayko]

This PR just removes the protection giving place to a potential data race.

[ethouris]

Closing: The analysis shows that this described locking is not a problem (m_ConnectionLock after m_LSLock is applied on a different socket). A description of this trick will be provided instead.