Inconsistencies/bugs with OpenMama


Sridharan, Venkatesan <Venkatesan.Sridharan@...>
 

Hi Folks,

This is my first post here. First off, thanks for your efforts on OpenMama, the code is clean, well commented and the documentation is great too!

We've been using OpenMama for a few months now and have written bridges for TibRv and Qpid (using qpid c++ API). I am talking to my company about opensourcing them. I came across a number of seeming errors/inconsistencies and wanted to get the community's take. I can provide a patch to fix the issues if we agree that they indeed are bugs and that they do need to be fixed.

* xxxxBridge_getDefaultPayloadId (char ***name, char **id)
The function retrieves the payload id which are const and hence params must be const qualified -> (const char ***name, const char **id).

* xxxPayload_getMsg(const msgPayload msg, const char* name, mama_fid_t fid, msgPayload* result)
We are passing in a const msg and extracting its field. so, last arg should be const msgPayload*

* loadbridge uses property file before open()
During creation of the default queue, properties subsystem is accessed and this always prints an error/warning if we use a custom properties file.

* bridgeMamaPublisherSendReplyToInbox => request is void* instead of mamaMsg
typedef mama_status
(*bridgeMamaPublisher_sendReplyToInbox)(publisherBridge publisher,
=>void * request, <=
mamaMsg reply);
This is needless as the request is a mamaMsg


* throw in dtor!
MamaTransport::~MamaTransport (void)
{
if (mPimpl)
{
if ( mamaInternal_getCatchCallbackExceptions() && mPimpl->mCallback)
{
delete mPimpl->mCallback;
}
if (mPimpl->mDeleteCTransport)
{
=>mamaTry (mamaTransport_destroy (mTransport)); <=
}
delete mPimpl;
mPimpl = NULL;
}
}

* Bridges are expected to allocate memory for some functions (toString, getReplyHandle etc) but there is no hook to free the memory. Since each bridge is allowed to use custom memory allocation, there should be a hook from mamaMsg_freeString back to the bridge. Also, currently mamaMsg_freeString is a no-op

* mamaMsg_copy does not check for self copy

* Some fields are not initialized in INITIALIZE_BRIDGE () macro. Eg, mamaBridgeImpl::mInternalDispatcher is not set to NULL on bridge creation. Got a crash on this one.

* As per comments and documentation, mamaQueue_timedDispatch is supposed to keep dispatching until the given timeout has elapsed. But, the current bridge implementations use
wombatQueue_timedDispatch which dispatches only one message. So, EITHER wombatQueue_timedDispatch should be changed to keep dispatching till timeout OR another helper function
should be provided to do this (as this functionality is used across bridges)

* msgPayload_unSerialize prototype incorrect (takes in void** instead of void*)
typedef mama_status
(*msgPayload_unSerialize) (const msgPayload msg,
=>const void** buffer, <=
mama_size_t bufferLength);
This is an input buffer and the only function using this is mamaMsg_setNewBuffer which calls the above function and passes a void* buffer as a void**

* Incorrect test in MamaMsg::setNewBuffer ():
bool MamaMsg::setNewBuffer (
void* buffer,
mama_size_t size)
{
int result = 0;
// should be MAMA_STATUS_OK == ...
=> if (MAMA_STATUS_OK != (mamaTry (mamaMsg_setNewBuffer ( <=
mMsg, buffer, size))))
{
return result;
}
return result != 0;
}

* These functions are not defined, I encountered this when trying to build python, perl interfaces with swig. A default implementation returning MAMA_STATUS_NOT_IMPLEMENTED or nullptr should be added
mamaMsg_updateVectorTime, mamaMsg_updateVectorPrice, mamaDispatcher_getQueue, mama_forceLogVaWithPrefix, mamaMsgField_getVectorBool, mama_logPolicyToString, mama_tryStringToLogPolicy, mamaMsgIterator_end

* Inconsistent prototypes :
mama_status
mamaMsg_updateVectorPrice (
mamaMsg msg,
const char* fname,
mama_fid_t fid,
//should be const mamaPrice priceList[]
=> const mamaPrice* priceList[], =<
mama_size_t numElements);

msgPayload_getPrice, msgPayload_getDateTime should take pointer to result as last parameter
Eg:
typedef mama_status
(*msgPayload_getDateTime) (const msgPayload msg,
const char* name,
mama_fid_t fid,
//should be mamaDateTime * result
=> mamaDateTime result); <=

* There are some pesky macros defined in port.h (close, sleep etc) which causes problems when using other libraries (eg boost). We should be defining them as inline functions instead of using macros


Cheers,
Venkatesan




Confidentiality Note: This e-mail and any attachments are confidential and may
be protected by legal privilege. If you are not the intended recipient, be aware
that any disclosure, copying, distribution or use of this e-mail or any
attachment is prohibited. If you have received this e-mail in error, please
notify us immediately by returning it to the sender and delete this copy from
your system. Thank you for your cooperation.


Frank Quinn <f.quinn@...>
 

Hi Venkatesan,

Glad you're getting something from the project and appreciate your efforts around this!

At first look, I agree in principle with a lot of the below, but I feel like it could get unruly trying to cover everything in a single thread. With that in mind, could you raise a github issue for each of the below so we can track each point in detail independently and make sure nothing slips through the net?

Also note there was also a series of patches submitted in the past to cover datetime / price vectors which may address at least some of your concerns, though we were holding off on implementing any bridge-related changes until dynamic bridge loading is completed:

http://lists.celinuxforum.org/pipermail/openmama-dev/2014-January/001002.html
http://lists.celinuxforum.org/pipermail/openmama-dev/2014-January/001003.html
http://lists.celinuxforum.org/pipermail/openmama-dev/2014-January/001004.html
http://lists.celinuxforum.org/pipermail/openmama-dev/2014-January/001005.html
http://lists.celinuxforum.org/pipermail/openmama-dev/2014-January/001006.html

Cheers,
Frank

-----Original Message-----
From: openmama-dev-bounces@lists.openmama.org [mailto:openmama-dev-bounces@lists.openmama.org] On Behalf Of Sridharan, Venkatesan
Sent: 12 October 2015 01:59
To: openmama-dev@lists.openmama.org
Subject: [Openmama-dev] Inconsistencies/bugs with OpenMama

Hi Folks,

This is my first post here. First off, thanks for your efforts on OpenMama, the code is clean, well commented and the documentation is great too!

We've been using OpenMama for a few months now and have written bridges for TibRv and Qpid (using qpid c++ API). I am talking to my company about opensourcing them. I came across a number of seeming errors/inconsistencies and wanted to get the community's take. I can provide a patch to fix the issues if we agree that they indeed are bugs and that they do need to be fixed.

* xxxxBridge_getDefaultPayloadId (char ***name, char **id) The function retrieves the payload id which are const and hence params must be const qualified -> (const char ***name, const char **id).

* xxxPayload_getMsg(const msgPayload msg, const char* name, mama_fid_t fid, msgPayload* result) We are passing in a const msg and extracting its field. so, last arg should be const msgPayload*

* loadbridge uses property file before open() During creation of the default queue, properties subsystem is accessed and this always prints an error/warning if we use a custom properties file.

* bridgeMamaPublisherSendReplyToInbox => request is void* instead of mamaMsg typedef mama_status (*bridgeMamaPublisher_sendReplyToInbox)(publisherBridge publisher,
=>void * request, <=
mamaMsg reply);
This is needless as the request is a mamaMsg


* throw in dtor!
MamaTransport::~MamaTransport (void)
{
if (mPimpl)
{
if ( mamaInternal_getCatchCallbackExceptions() && mPimpl->mCallback)
{
delete mPimpl->mCallback;
}
if (mPimpl->mDeleteCTransport)
{
=>mamaTry (mamaTransport_destroy (mTransport)); <=
}
delete mPimpl;
mPimpl = NULL;
}
}

* Bridges are expected to allocate memory for some functions (toString, getReplyHandle etc) but there is no hook to free the memory. Since each bridge is allowed to use custom memory allocation, there should be a hook from mamaMsg_freeString back to the bridge. Also, currently mamaMsg_freeString is a no-op

* mamaMsg_copy does not check for self copy

* Some fields are not initialized in INITIALIZE_BRIDGE () macro. Eg, mamaBridgeImpl::mInternalDispatcher is not set to NULL on bridge creation. Got a crash on this one.

* As per comments and documentation, mamaQueue_timedDispatch is supposed to keep dispatching until the given timeout has elapsed. But, the current bridge implementations use wombatQueue_timedDispatch which dispatches only one message. So, EITHER wombatQueue_timedDispatch should be changed to keep dispatching till timeout OR another helper function should be provided to do this (as this functionality is used across bridges)

* msgPayload_unSerialize prototype incorrect (takes in void** instead of void*) typedef mama_status
(*msgPayload_unSerialize) (const msgPayload msg,
=>const void** buffer, <=
mama_size_t bufferLength);
This is an input buffer and the only function using this is mamaMsg_setNewBuffer which calls the above function and passes a void* buffer as a void**

* Incorrect test in MamaMsg::setNewBuffer ():
bool MamaMsg::setNewBuffer (
void* buffer,
mama_size_t size)
{
int result = 0;
// should be MAMA_STATUS_OK == ...
=> if (MAMA_STATUS_OK != (mamaTry (mamaMsg_setNewBuffer ( <=
mMsg, buffer, size))))
{
return result;
}
return result != 0;
}

* These functions are not defined, I encountered this when trying to build python, perl interfaces with swig. A default implementation returning MAMA_STATUS_NOT_IMPLEMENTED or nullptr should be added mamaMsg_updateVectorTime, mamaMsg_updateVectorPrice, mamaDispatcher_getQueue, mama_forceLogVaWithPrefix, mamaMsgField_getVectorBool, mama_logPolicyToString, mama_tryStringToLogPolicy, mamaMsgIterator_end

* Inconsistent prototypes :
mama_status
mamaMsg_updateVectorPrice (
mamaMsg msg,
const char* fname,
mama_fid_t fid,
//should be const mamaPrice priceList[]
=> const mamaPrice* priceList[], =<
mama_size_t numElements);

msgPayload_getPrice, msgPayload_getDateTime should take pointer to result as last parameter
Eg:
typedef mama_status
(*msgPayload_getDateTime) (const msgPayload msg,
const char* name,
mama_fid_t fid,
//should be mamaDateTime * result
=> mamaDateTime result); <=

* There are some pesky macros defined in port.h (close, sleep etc) which causes problems when using other libraries (eg boost). We should be defining them as inline functions instead of using macros


Cheers,
Venkatesan




Confidentiality Note: This e-mail and any attachments are confidential and may be protected by legal privilege. If you are not the intended recipient, be aware that any disclosure, copying, distribution or use of this e-mail or any attachment is prohibited. If you have received this e-mail in error, please notify us immediately by returning it to the sender and delete this copy from your system. Thank you for your cooperation.
_______________________________________________
Openmama-dev mailing list
Openmama-dev@lists.openmama.org
https://lists.openmama.org/mailman/listinfo/openmama-dev

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. SR Labs LLC


Sridharan, Venkatesan <Venkatesan.Sridharan@...>
 

Thanks Frank. Have raised a github issue for each point.

-----Original Message-----
From: Frank Quinn [mailto:f.quinn@srtechlabs.com]
Sent: Monday, October 12, 2015 3:22 PM
To: Sridharan, Venkatesan <Venkatesan.Sridharan@Squarepoint-Capital.com>; openmama-dev@lists.openmama.org
Subject: RE: Inconsistencies/bugs with OpenMama

Hi Venkatesan,

Glad you're getting something from the project and appreciate your efforts around this!

At first look, I agree in principle with a lot of the below, but I feel like it could get unruly trying to cover everything in a single thread. With that in mind, could you raise a github issue for each of the below so we can track each point in detail independently and make sure nothing slips through the net?

Also note there was also a series of patches submitted in the past to cover datetime / price vectors which may address at least some of your concerns, though we were holding off on implementing any bridge-related changes until dynamic bridge loading is completed:

http://lists.celinuxforum.org/pipermail/openmama-dev/2014-January/001002.html
http://lists.celinuxforum.org/pipermail/openmama-dev/2014-January/001003.html
http://lists.celinuxforum.org/pipermail/openmama-dev/2014-January/001004.html
http://lists.celinuxforum.org/pipermail/openmama-dev/2014-January/001005.html
http://lists.celinuxforum.org/pipermail/openmama-dev/2014-January/001006.html

Cheers,
Frank

-----Original Message-----
From: openmama-dev-bounces@lists.openmama.org [mailto:openmama-dev-bounces@lists.openmama.org] On Behalf Of Sridharan, Venkatesan
Sent: 12 October 2015 01:59
To: openmama-dev@lists.openmama.org
Subject: [Openmama-dev] Inconsistencies/bugs with OpenMama

Hi Folks,

This is my first post here. First off, thanks for your efforts on OpenMama, the code is clean, well commented and the documentation is great too!

We've been using OpenMama for a few months now and have written bridges for TibRv and Qpid (using qpid c++ API). I am talking to my company about opensourcing them. I came across a number of seeming errors/inconsistencies and wanted to get the community's take. I can provide a patch to fix the issues if we agree that they indeed are bugs and that they do need to be fixed.

* xxxxBridge_getDefaultPayloadId (char ***name, char **id) The function retrieves the payload id which are const and hence params must be const qualified -> (const char ***name, const char **id).

* xxxPayload_getMsg(const msgPayload msg, const char* name, mama_fid_t fid, msgPayload* result) We are passing in a const msg and extracting its field. so, last arg should be const msgPayload*

* loadbridge uses property file before open() During creation of the default queue, properties subsystem is accessed and this always prints an error/warning if we use a custom properties file.

* bridgeMamaPublisherSendReplyToInbox => request is void* instead of mamaMsg typedef mama_status (*bridgeMamaPublisher_sendReplyToInbox)(publisherBridge publisher,
=>void * request, <=
mamaMsg reply);
This is needless as the request is a mamaMsg


* throw in dtor!
MamaTransport::~MamaTransport (void)
{
if (mPimpl)
{
if ( mamaInternal_getCatchCallbackExceptions() && mPimpl->mCallback)
{
delete mPimpl->mCallback;
}
if (mPimpl->mDeleteCTransport)
{
=>mamaTry (mamaTransport_destroy (mTransport)); <=
}
delete mPimpl;
mPimpl = NULL;
}
}

* Bridges are expected to allocate memory for some functions (toString, getReplyHandle etc) but there is no hook to free the memory. Since each bridge is allowed to use custom memory allocation, there should be a hook from mamaMsg_freeString back to the bridge. Also, currently mamaMsg_freeString is a no-op

* mamaMsg_copy does not check for self copy

* Some fields are not initialized in INITIALIZE_BRIDGE () macro. Eg, mamaBridgeImpl::mInternalDispatcher is not set to NULL on bridge creation. Got a crash on this one.

* As per comments and documentation, mamaQueue_timedDispatch is supposed to keep dispatching until the given timeout has elapsed. But, the current bridge implementations use wombatQueue_timedDispatch which dispatches only one message. So, EITHER wombatQueue_timedDispatch should be changed to keep dispatching till timeout OR another helper function should be provided to do this (as this functionality is used across bridges)

* msgPayload_unSerialize prototype incorrect (takes in void** instead of void*) typedef mama_status
(*msgPayload_unSerialize) (const msgPayload msg,
=>const void** buffer, <=
mama_size_t bufferLength);
This is an input buffer and the only function using this is mamaMsg_setNewBuffer which calls the above function and passes a void* buffer as a void**

* Incorrect test in MamaMsg::setNewBuffer ():
bool MamaMsg::setNewBuffer (
void* buffer,
mama_size_t size)
{
int result = 0;
// should be MAMA_STATUS_OK == ...
=> if (MAMA_STATUS_OK != (mamaTry (mamaMsg_setNewBuffer ( <=
mMsg, buffer, size))))
{
return result;
}
return result != 0;
}

* These functions are not defined, I encountered this when trying to build python, perl interfaces with swig. A default implementation returning MAMA_STATUS_NOT_IMPLEMENTED or nullptr should be added mamaMsg_updateVectorTime, mamaMsg_updateVectorPrice, mamaDispatcher_getQueue, mama_forceLogVaWithPrefix, mamaMsgField_getVectorBool, mama_logPolicyToString, mama_tryStringToLogPolicy, mamaMsgIterator_end

* Inconsistent prototypes :
mama_status
mamaMsg_updateVectorPrice (
mamaMsg msg,
const char* fname,
mama_fid_t fid,
//should be const mamaPrice priceList[]
=> const mamaPrice* priceList[], =<
mama_size_t numElements);

msgPayload_getPrice, msgPayload_getDateTime should take pointer to result as last parameter
Eg:
typedef mama_status
(*msgPayload_getDateTime) (const msgPayload msg,
const char* name,
mama_fid_t fid,
//should be mamaDateTime * result
=> mamaDateTime result); <=

* There are some pesky macros defined in port.h (close, sleep etc) which causes problems when using other libraries (eg boost). We should be defining them as inline functions instead of using macros


Cheers,
Venkatesan




Confidentiality Note: This e-mail and any attachments are confidential and may be protected by legal privilege. If you are not the intended recipient, be aware that any disclosure, copying, distribution or use of this e-mail or any attachment is prohibited. If you have received this e-mail in error, please notify us immediately by returning it to the sender and delete this copy from your system. Thank you for your cooperation.
_______________________________________________
Openmama-dev mailing list
Openmama-dev@lists.openmama.org
https://lists.openmama.org/mailman/listinfo/openmama-dev

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. SR Labs LLC



Confidentiality Note: This e-mail and any attachments are confidential and may
be protected by legal privilege. If you are not the intended recipient, be aware
that any disclosure, copying, distribution or use of this e-mail or any
attachment is prohibited. If you have received this e-mail in error, please
notify us immediately by returning it to the sender and delete this copy from
your system. Thank you for your cooperation.