Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid accessing a null ClientConnection instance #317

Merged

Commits on Sep 19, 2023

  1. Avoid accessing a null ClientConnection instance

    ### Motivation
    
    We observed server null `ClientConnection` accesses in test environment.
    See the `this=0x0` outputs in the following two typical stacks.
    
    ```
    #8  bytesWritten (this=0xb8, size=371) at lib/SharedBuffer.h:166
    #9  pulsar::ClientConnection::handleRead (this=0x0, err=..., bytesTransferred=371, minReadSize=4) at lib/ClientConnection.cc:609
    ```
    
    ```
    #12 0x00007f33202933d2 in unique_lock (__m=..., this=0x7f3311c82800) at /opt/rh/devtoolset-7/root/usr/include/c++/7/bits/std_mutex.h:197
    #13 pulsar::ClientConnection::sendPendingCommands (this=0x0) at lib/ClientConnection.cc:1071
    #14 0x00007f3320293d2d in pulsar::ClientConnection::handleSendPair (this=0x0, err=...) at lib/ClientConnection.cc:1066
    ```
    
    Though `shared_from_this()` is always passed to the `std::bind`
    function, when the method of `ClientConnection` is called, the pointer
    is still `null`.
    
    ### Modifications
    
    First, replace all `std::bind` calls with the lambda expression that
    catches `std::weak_ptr<ClientConnection>` and perform null checks
    explicitly on the value returned by the `lock()` method.
    
    Since now all asio callbacks don't hold a `shared_ptr`, the owner of
    the `ClientConnection` object should be `ConnectionPool`, i.e. the pool
    maintains some connections, while all asio callbacks use `weak_ptr` to
    test if the connection is present.
    
    Second, make `ClientConnection::getConnection` return `shared_ptr`
    rather than `weak_ptr` so that the caller side does not need to check if
    `lock()` returns null in the callback of this future.
    
    We cannot make `ConnectionPool::getConnectionAsync` return `shared_ptr`
    because it could return the future of `connectPromise_`, which is hold
    by `ClientConnection` itself. We should avoid holding a `shared_ptr` of
    `ClientConnection` because its owner is `ConnectionPool`.
    BewareMyPower committed Sep 19, 2023
    Configuration menu
    Copy the full SHA
    52e1531 View commit details
    Browse the repository at this point in the history