From dev-return-32009-apmail-qpid-dev-archive=qpid.apache.org@qpid.apache.org Sun Dec 18 21:54:57 2011 Return-Path: X-Original-To: apmail-qpid-dev-archive@www.apache.org Delivered-To: apmail-qpid-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id C14259A63 for ; Sun, 18 Dec 2011 21:54:57 +0000 (UTC) Received: (qmail 81524 invoked by uid 500); 18 Dec 2011 21:54:57 -0000 Delivered-To: apmail-qpid-dev-archive@qpid.apache.org Received: (qmail 81418 invoked by uid 500); 18 Dec 2011 21:54:57 -0000 Mailing-List: contact dev-help@qpid.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@qpid.apache.org Delivered-To: mailing list dev@qpid.apache.org Received: (qmail 81410 invoked by uid 99); 18 Dec 2011 21:54:57 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 18 Dec 2011 21:54:57 +0000 X-ASF-Spam-Status: No, hits=-2002.5 required=5.0 tests=ALL_TRUSTED,RP_MATCHES_RCVD X-Spam-Check-By: apache.org Received: from [140.211.11.116] (HELO hel.zones.apache.org) (140.211.11.116) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 18 Dec 2011 21:54:54 +0000 Received: from hel.zones.apache.org (hel.zones.apache.org [140.211.11.116]) by hel.zones.apache.org (Postfix) with ESMTP id 24C2B11ACC9 for ; Sun, 18 Dec 2011 21:54:32 +0000 (UTC) Date: Sun, 18 Dec 2011 21:54:32 +0000 (UTC) From: "jiraposter@reviews.apache.org (Commented) (JIRA)" To: dev@qpid.apache.org Message-ID: <1773154215.24630.1324245272172.JavaMail.tomcat@hel.zones.apache.org> In-Reply-To: <1407306065.42243.1302210965759.JavaMail.tomcat@hel.zones.apache.org> Subject: [jira] [Commented] (QPID-3193) .NET Binding for Messaging classes need a test to check that binding is still in effect MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-JIRA-FingerPrint: 30527f35849b9dde25b450d4833f0394 X-Virus-Checked: Checked by ClamAV on apache.org [ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13171945#comment-13171945 ] jiraposter@reviews.apache.org commented on QPID-3193: ----------------------------------------------------- ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3239/#review3968 ----------------------------------------------------------- This is definitely moving in the right direction. I would request 3 changes. The changes you made to the use of destructors and finalizers are less correct than the original code. You are duplicating the work of the IDispose wrapper created by the compiler. See "Destructors and Finalizers in Visual C++" in the Microsoft documentation where it says "Calling the destructor will suppress (with SuppressFinalize) finalization of the object". The need for an explicit GC::SuppressFinalize(this) is only needed if there is an additional mechanism from the IDisposable interface to allow user control of the resources. As an example, see AmqpConnection::Close() in the Qpid wcf code. Regarding your choice of "this" for locking: WRT lock(this), I think it is the best choice. The entire Message class storage consists of a single pointer into unmanaged space. There is no finer-grained object on which to lock than 'this'. I disagree. The provided library is at risk of deadlock due to sloppy user code or inadvertent user error. You should use something along the lines of: Object^ wrapperLock; // private, in .h file wrapperLock = gcnew Object(); // in constructors See the openCloseLock in the AmqpSession class in the Qpid wcf code for an example where more locks were needed than available private objects. Or see the use of thisLock in IssuedTokenCache.cs in the WCF examples from Microsoft. The Macro: I agree with Andrew that the side effects of the macro should be clearly pointed out. One example, not using a macro, is the use of CheckOpen() in the wcf client's AmqpSession class (and s/CheckOpen/ThrowIfDisposed/ for your case). This is probably how most .NET programmers would approach it since they tend not to prefer compactly written code over descriptive code. However I am sympathetic to the magnitude of intrusion of the change to your wrapper class which suddenly makes your code appear to be as much about mutithreading as it is about its real job. Given that the complexities of the managed/unmanaged boundary dwarf your intended use of the macro, I don't think any one working with this code would find the macro confusing. If you prefer a macro, that works for me, provided you address Andrew's concern and switch to locking on a private reference. Cliff - Cliff On 2011-12-16 21:04:58, Chug Rolke wrote: bq. bq. ----------------------------------------------------------- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/3239/ bq. ----------------------------------------------------------- bq. bq. (Updated 2011-12-16 21:04:58) bq. bq. bq. Review request for qpid, Andrew Stitcher, Gordon Sim, Ted Ross, Steve Huston, and Cliff Jansen. bq. bq. bq. Summary bq. ------- bq. bq. QPID-3193 .NET Binding - proper handling of disposed objects. bq. bq. bq. This addresses bug QPID-3193. bq. https://issues.apache.org/jira/browse/QPID-3193 bq. bq. bq. Diffs bq. ----- bq. bq. trunk/qpid/cpp/bindings/qpid/dotnet/src/BindingCommon.h PRE-CREATION bq. trunk/qpid/cpp/bindings/qpid/dotnet/src/Message.h 1215245 bq. trunk/qpid/cpp/bindings/qpid/dotnet/src/Message.cpp 1215245 bq. trunk/qpid/cpp/bindings/qpid/dotnet/src/msvc10/org.apache.qpid.messaging.vcxproj 1215245 bq. trunk/qpid/cpp/bindings/qpid/dotnet/src/msvc9/org.apache.qpid.messaging.vcproj 1215245 bq. bq. Diff: https://reviews.apache.org/r/3239/diff bq. bq. bq. Testing bq. ------- bq. bq. bq. Thanks, bq. bq. Chug bq. bq. > .NET Binding for Messaging classes need a test to check that binding is still in effect > --------------------------------------------------------------------------------------- > > Key: QPID-3193 > URL: https://issues.apache.org/jira/browse/QPID-3193 > Project: Qpid > Issue Type: Improvement > Affects Versions: 0.11 > Reporter: Chuck Rolke > Assignee: Chuck Rolke > Attachments: QPID-3694_lock-and-throw-preview.patch > > > The .NET Binding for Messaging could be made more user-friendly with the addition of a property that indicates whether or not the underlying binding is still available. A C# coder may innocently write: > (1) Message mA = new Message("a"); > (2) Message mB = mA; > ... > (N) mB.Dispose(); > After disposing of message mB then message mA is clobbered. 'Message' is a 'ref class' type and messages mA and mB refer to the same object on managed heap. When message mB is disposed then the bound C++ Messaging object is deleted [1]. Any reference to the bound message part of mA will result in an illegal memory reference (to 0) and a process exit. The .NET runtime can't catch this fault. > The obvious answer is not to do that. If the second line of code was 'Message mB = new Message(mA)' then mA and mB would have been completely separate and disposing of either would have no effect on the other. > Another answer is to have the binding check for a null binding reference on each and every access and then to throw if the underlying binding is gone. This is not very appealing from a performance standpoint. > As a compromise I would like to add a property isBound to each class. Users then have a fighting chance to check that the binding is still in effect and that function calls on the object shouldn't blow up. This property would be useful in Assert statements or in debugging. > [1] If anyone knows how to have my binding library intercept example code line (2) and create reference counts, please let me know. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira --------------------------------------------------------------------- Apache Qpid - AMQP Messaging Implementation Project: http://qpid.apache.org Use/Interact: mailto:dev-subscribe@qpid.apache.org