Questions about MamaStartCallback


Yury Batrakov
 

Classification: Public

Hi guys,

As everybody knows there is possibility to start MAMA bridge in background thread in C++ API: function Mama::startBackground (mamaBridge bridgeImpl, MamaStartCallback* cb)
There are a couple of problems with this function though:
1. We can't ignore "MAMA startup finished" event setting the value for the second argument to nullptr - this can be fixed as a simple nullptr check in void MAMACALLTYPE stopCb (mama_status status, mamaBridge, void* closure) in mamacpp.cpp
2. There is race condition between thread calling Mama::stop and internal MAMA thread spawned by startBackground function. This race may lead to program crash.

Let me describe #2 - consider the following scenario:

We have a simple class MyMamaConnection

class MyMamaStartCallback : public MamaStartCallback {
void onStartComplete (Wombat::MamaStatus status) override {
// .... Some bridge specific callback code
}
};

class MyMamaConnection {
mamaBridge m_bridge;
MyMamaStartCallback m_callback;

MyMamaConnection() {
Mama::startBackground (m_bridge, &m_callback)
}

~MyMamaConnection() {
Mama::stop(m_bridge);
}
};

The crash occur in destructor because Mama::stop doesn't guarantee that no threads are still using m_callback pointer after this function exited. In our case MyMamaConnection's destructor destroys m_callback object while it is still in use by the thread created by Mama::startBackground.

Here are more details, file mama.c:

static void* mamaStartThread (void* closure)
{
...
rval = mama_start (cb->mBridgeImpl); // Imagine user thread Thread_1 calling ~MyMamaConnection() from our example.

// Current thread may be suspended by OS at this point while Thread_1 continues its work and deletes the object pointed by closure
...

if (cb->mStopCallbackEx)

// cb->mClosure points to destroyed object.

cb->mStopCallbackEx (rval, cb->mBridgeImpl, cb->mClosure);
...
}

One of the possible solutions is to move this code from mama_close() to mama_stop():
// Get name of start background thread and destroy it
const char* threadName = wombatThread_getThreadName(middlewareLib->bridge->mStartBackgroundThread);
wombatThread_destroy (threadName);

Let me know if it's possible to have this fixed in the next release.


---
This e-mail may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and delete this e-mail. Any unauthorized copying, disclosure or distribution of the material in this e-mail is strictly forbidden.

Please refer to https://www.db.com/disclosures for additional EU corporate and regulatory disclosures and to http://www.db.com/unitedkingdom/content/privacy.htm for information about privacy.


Frank Quinn <fquinn@...>
 

Thanks Yury,

Yes that analysis and suggestion both sound correct. I just raised https://github.com/OpenMAMA/OpenMAMA/issues/319 for tracking so it will make it into the next release.

Cheers,
Frank


FRANK QUINN
Principal Engineer
Vela Trading Technologies

O. +44 289 568 0209 ext. 3592
fquinn@...

Adelaide Exchange Building, 2nd Floor, 24-26 Adelaide Street, Belfast, BT2 8GD
velatradingtech.com | @vela_tt

-----Original Message-----
From: Yury Batrakov [mailto:yury.batrakov@...]
Sent: 14 August 2017 13:21
To: openmama-users <openmama-users@...>; openmama-dev <openmama-dev@...>
Cc: Frank Quinn <fquinn@...>; Marc Alzina <marc.alzina@...>
Subject: Questions about MamaStartCallback

Classification: Public

Hi guys,

As everybody knows there is possibility to start MAMA bridge in background thread in C++ API: function Mama::startBackground (mamaBridge bridgeImpl, MamaStartCallback* cb) There are a couple of problems with this function though:
1. We can't ignore "MAMA startup finished" event setting the value for the second argument to nullptr - this can be fixed as a simple nullptr check in void MAMACALLTYPE stopCb (mama_status status, mamaBridge, void* closure) in mamacpp.cpp
2. There is race condition between thread calling Mama::stop and internal MAMA thread spawned by startBackground function. This race may lead to program crash.

Let me describe #2 - consider the following scenario:

We have a simple class MyMamaConnection

class MyMamaStartCallback : public MamaStartCallback {
void onStartComplete (Wombat::MamaStatus status) override {
// .... Some bridge specific callback code
}
};

class MyMamaConnection {
mamaBridge m_bridge;
MyMamaStartCallback m_callback;

MyMamaConnection() {
Mama::startBackground (m_bridge, &m_callback)
}

~MyMamaConnection() {
Mama::stop(m_bridge);
}
};

The crash occur in destructor because Mama::stop doesn't guarantee that no threads are still using m_callback pointer after this function exited. In our case MyMamaConnection's destructor destroys m_callback object while it is still in use by the thread created by Mama::startBackground.

Here are more details, file mama.c:

static void* mamaStartThread (void* closure) { ...
rval = mama_start (cb->mBridgeImpl); // Imagine user thread Thread_1 calling ~MyMamaConnection() from our example.

// Current thread may be suspended by OS at this point while Thread_1 continues its work and deletes the object pointed by closure ...

if (cb->mStopCallbackEx)

// cb->mClosure points to destroyed object.

cb->mStopCallbackEx (rval, cb->mBridgeImpl, cb->mClosure); ...
}

One of the possible solutions is to move this code from mama_close() to mama_stop():
// Get name of start background thread and destroy it
const char* threadName = wombatThread_getThreadName(middlewareLib->bridge->mStartBackgroundThread);
wombatThread_destroy (threadName);

Let me know if it's possible to have this fixed in the next release.


---
This e-mail may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and delete this e-mail. Any unauthorized copying, disclosure or distribution of the material in this e-mail is strictly forbidden.

Please refer to https://www.db.com/disclosures for additional EU corporate and regulatory disclosures and to http://www.db.com/unitedkingdom/content/privacy.htm for information about privacy.

The information contained in this message may be privileged and confidential and protected from disclosure. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please notify us immediately by replying to this message and deleting it from your computer. Thank you. Vela Trading Technologies LLC


Frank Quinn <fquinn.ni@...>
 

Hi Yury,

I did have this proposed change merged, but have since discovered that it causes a deadlock in our unit test on windows.

On closer inspection, it looks like the problem is caused by the fact that making mama_stop blocking will effectively deadlock any application which attempts to call mama_stop from a MAMA event coming off the default queue (like our unit test application). Since I expect this is a common enough pattern, we'll have to revert this change.

Note that the argument to startBackground is misleadingly a stop callback - i.e. it will be called when mama_start *unblocks*. With this in mind, you should be able to get your MyMamaConnection destructor to wait on a semaphore raised by onStartComplete method (use mama_startBackgroundEx rather than mama_startBackground to provide a closure) to raise a semaphore to simulate the equivalent behaviour and prevent the crash.

Cheers,
Frank

On Mon, Aug 14, 2017 at 1:21 PM, Yury Batrakov <yury.batrakov@...> wrote:
Classification: Public

Hi guys,

As everybody knows there is possibility to start MAMA bridge in background thread in C++ API: function Mama::startBackground (mamaBridge bridgeImpl, MamaStartCallback* cb)
There are a couple of problems with this function though:
    1. We can't ignore "MAMA startup finished" event setting  the value for the second argument to nullptr - this can be fixed as a simple nullptr check in void MAMACALLTYPE stopCb (mama_status status, mamaBridge, void* closure) in mamacpp.cpp
    2. There is race condition between thread calling Mama::stop and internal MAMA thread spawned  by startBackground function. This race may lead to program crash.

Let me describe #2 - consider the following scenario:

We have a simple class MyMamaConnection

class MyMamaStartCallback : public MamaStartCallback {
    void onStartComplete (Wombat::MamaStatus status) override {
           // .... Some bridge specific callback code
    }
};

class MyMamaConnection {
    mamaBridge m_bridge;
    MyMamaStartCallback m_callback;

    MyMamaConnection() {
        Mama::startBackground (m_bridge, &m_callback)
    }

    ~MyMamaConnection() {
        Mama::stop(m_bridge);
    }
};

The crash occur in destructor because Mama::stop doesn't guarantee that no threads are still using m_callback pointer after this function exited. In our case MyMamaConnection's destructor destroys m_callback object while it is still in use by the thread created by Mama::startBackground.

Here are more details, file mama.c:

static void* mamaStartThread (void* closure)
{
...
    rval = mama_start (cb->mBridgeImpl); // Imagine user thread Thread_1 calling ~MyMamaConnection() from our example.

    // Current thread may be suspended by OS at this point while Thread_1 continues its work and deletes the object pointed by closure
...

    if (cb->mStopCallbackEx)

        // cb->mClosure points to destroyed object.

        cb->mStopCallbackEx (rval, cb->mBridgeImpl, cb->mClosure);
...
}

One of the possible solutions is to move this code from mama_close() to mama_stop():
                        // Get name of start background thread and destroy it
                        const char* threadName = wombatThread_getThreadName(middlewareLib->bridge->mStartBackgroundThread);
                        wombatThread_destroy (threadName);

Let me know if it's possible to have this fixed in the next release.


---
This e-mail may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and delete this e-mail. Any unauthorized copying, disclosure or distribution of the material in this e-mail is strictly forbidden.

Please refer to https://www.db.com/disclosures for additional EU corporate and regulatory disclosures and to http://www.db.com/unitedkingdom/content/privacy.htm for information about privacy.
_______________________________________________
Openmama-dev mailing list
Openmama-dev@....org
https://lists.openmama.org/mailman/listinfo/openmama-dev