qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Cliff Jansen" <cliffjan...@gmail.com>
Subject Re: Review Request: QPID-3193 .NET Binding - handling disposed objects
Date Sun, 18 Dec 2011 21:53:31 GMT

-----------------------------------------------------------
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:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3239/
> -----------------------------------------------------------
> 
> (Updated 2011-12-16 21:04:58)
> 
> 
> Review request for qpid, Andrew Stitcher, Gordon Sim, Ted Ross, Steve Huston, and Cliff
Jansen.
> 
> 
> Summary
> -------
> 
> QPID-3193 .NET Binding - proper handling of disposed objects.
> 
> 
> This addresses bug QPID-3193.
>     https://issues.apache.org/jira/browse/QPID-3193
> 
> 
> Diffs
> -----
> 
>   trunk/qpid/cpp/bindings/qpid/dotnet/src/BindingCommon.h PRE-CREATION 
>   trunk/qpid/cpp/bindings/qpid/dotnet/src/Message.h 1215245 
>   trunk/qpid/cpp/bindings/qpid/dotnet/src/Message.cpp 1215245 
>   trunk/qpid/cpp/bindings/qpid/dotnet/src/msvc10/org.apache.qpid.messaging.vcxproj 1215245

>   trunk/qpid/cpp/bindings/qpid/dotnet/src/msvc9/org.apache.qpid.messaging.vcproj 1215245

> 
> Diff: https://reviews.apache.org/r/3239/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Chug
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message