[jira] Commented: (QPID-2589) Add a .NET binding to QPID Messaging API

["Cliff Jansen (JIRA)" <qp...@incubator.apache.org>]

    [ https://issues.apache.org/jira/browse/QPID-2589?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12865861#action_12865861 ] 

Cliff Jansen commented on QPID-2589:
------------------------------------

This new C# API has good potential.  I worry though about the choice of System.String as your starting point.  Perhaps you meant to add eventually new constructors and methods to also match the existing C++:

    QPID_CLIENT_EXTERN Message(const char*, size_t);
    QPID_CLIENT_EXTERN void setContent(const char* chars, size_t count);

in which case I think .NET folks would appreciate two options for each:

    Message(byte[] bytes);
    Message(byte[] bytes, int index, int count);
    SetContent(byte[] bytes)
    SetContent(byte[] bytes, int index, int count);

In any event, it makes more sense that the System.String content be accessed or set via an encoder, as you probably intend to do for maps and other complex things.  Keep in mind that std::string are a reasonable choice for manipulating binary content in C++.  Trying to do the same thing with *immutable* Strings of 16 bit data is likely to cause severe brain strain during your implementation and .NET users won't thank you for your trouble.


You might also want to tone down the C++-ness of the API, for example by using .NET naming conventions (i.e. case for methods), and using properties instead of methods for things like getMilliseconds() or setSubject().


As a C++/CLI tip, be more defensive when passing responsibility for cleanup of native objects.  I added the try block to your code

    Sender ^ Session::createSender  (System::String ^ address)
    {
        // allocate a native sender
        ::qpid::messaging::Sender * senderp = new ::qpid::messaging::Sender;

	try {
          // create the sender
          *senderp = sessionp->::qpid::messaging::Session::createSender(QpidMarshal::ToNative(address));

          // create a managed sender
          Sender ^ newSender = gcnew Sender(senderp, this);

          senderp = NULL;
        }
        finally {
	  if (senderp !=NULL) { 
            delete senderp
          }
        }

        return newSender;
    }

Otherwise, if there is an exception in createSender() there is a memory leak.  If there is an exception in the Sender() constructor, it is worse.  There will be no finalizer call since the constructor didn't complete, so you wonder did it catch the exception and delete the native resource?  The solution is for the receiver of the native object, Sender in this case, to take responsibility for cleanup only as the last thing before returning when an exception is no longer possible.  In this particular case it works out that way because there are only memory assignments to existing memory.  You should look at all the other cases where you do a similar handoff.
    


> Add a .NET binding to QPID Messaging API
> ----------------------------------------
>
>                 Key: QPID-2589
>                 URL: https://issues.apache.org/jira/browse/QPID-2589
>             Project: Qpid
>          Issue Type: New Feature
>          Components: C++ Client
>    Affects Versions: 0.7
>         Environment: Windows
>            Reporter: Chuck Rolke
>            Assignee: Ted Ross
>             Fix For: 0.7
>
>         Attachments: qpid_bindings.diff
>
>
> This binding package is a .NET Interop wrapper around the Qpid Messaging interface. It exposes the Messaging interface through a series of managed code classes that may be used by any .NET language.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project:      http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org