You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by "Chuck Rolke (JIRA)" <ji...@apache.org> on 2011/04/07 23:16:05 UTC
[jira] [Created] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
.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
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.
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
[jira] [Commented] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13171193#comment-13171193 ]
jiraposter@reviews.apache.org commented on QPID-3193:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3239/
-----------------------------------------------------------
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
> .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
[jira] [Commented] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
Posted by "Cliff Jansen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13017239#comment-13017239 ]
Cliff Jansen commented on QPID-3193:
------------------------------------
I think you are approaching this the wrong way.
As the owner of the managed/unmanaged interface, it is your responsibility to catch and trap exceptions that the user of your class can't handle. Changing the semantics of Dispose() is not what the user would expect.
> A C# coder may innocently write:
[...]
> (N) mB.Dispose();
The sample that you show works exactly as it should. Replace your Message class with System.IO.Stream (or any other IDisposable class), and it will also fail when you try to access a feature that is undefined for the disposed object.
Conversely, if you faked the dispose because you somehow knew mA still wanted it, you would have robbed mB of it's attempt to optimize the use of the unmanaged resource.
If you absolutely need to to let the application know if the message is disposed, see
http://stackoverflow.com/questions/192206/how-does-one-tell-if-an-idisposable-object-reference-is-disposed
but it is rarely needed, otherwise it would be a commonly used pattern. The user of mA would usually just use a try catch block if he is worried about an async dispose since:
if (!mA.IsDisposed) {
// another thread calls mB.Dispose() or mA.Dispose()
mA.DoSomething();
}
still leaves a window open.
> 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.
If your class is meant to be thread safe, you have to take a deeper performance hit and acquire a lock for the duration of test and use. If you don't need thread safety, to avoid the test except on failure use:
try {
return messagep->foo();
}
catch (const std::exception& error) {
if (someDisposedTestHere) {
throw gcnew ObjectDisposedException("Message foo");
else
throw gcnew MyException(gcnew String(error.what()));
}
I believe this should convert the null pointer (unmaged space) exception for the user into the expected managed space exception.
> .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
>
> 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.
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
[jira] [Issue Comment Edited] (QPID-3193) .NET Binding for
Messaging classes need a test to check that binding is still in effect
Posted by "Cliff Jansen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13017239#comment-13017239 ]
Cliff Jansen edited comment on QPID-3193 at 4/8/11 2:28 AM:
------------------------------------------------------------
I think you are approaching this the wrong way.
As the owner of the managed/unmanaged interface, it is your responsibility to catch and trap exceptions that the user of your class can't handle. Changing the semantics of Dispose() is not what the user would expect.
> A C# coder may innocently write:
[...]
> (N) mB.Dispose();
The sample that you show works exactly as it should. Replace your Message class with System.IO.Stream (or any other IDisposable class), and it will also fail when you try to access a feature that is undefined for the disposed object.
Conversely, if you faked the dispose because you somehow knew mA still wanted it, you would have robbed mB of it's attempt to optimize the use of the unmanaged resource.
If you absolutely need to to let the application know if the message is disposed, see
http://stackoverflow.com/questions/192206/how-does-one-tell-if-an-idisposable-object-reference-is-disposed
but it is rarely needed, otherwise it would be a commonly used pattern. The user of mA would usually just use a try catch block if he is worried about an async dispose since:
{code}
if (!mA.IsDisposed) {
// another thread calls mB.Dispose() or mA.Dispose()
mA.DoSomething();
}
{code}
still leaves a window open.
> 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.
If your class is meant to be thread safe, you have to take a deeper performance hit and acquire a lock for the duration of test and use. If you don't need thread safety, to avoid the test except on failure use:
{code}
try {
return messagep->foo();
}
catch (const std::exception& error) {
if (someDisposedTestHere) {
throw gcnew ObjectDisposedException("Message foo");
else
throw gcnew MyException(gcnew String(error.what()));
}
{code}
I believe this should convert the null pointer (unmaged space) exception for the user into the expected managed space exception.
[sorry, forgot the code tags]
was (Author: cliffjansen):
I think you are approaching this the wrong way.
As the owner of the managed/unmanaged interface, it is your responsibility to catch and trap exceptions that the user of your class can't handle. Changing the semantics of Dispose() is not what the user would expect.
> A C# coder may innocently write:
[...]
> (N) mB.Dispose();
The sample that you show works exactly as it should. Replace your Message class with System.IO.Stream (or any other IDisposable class), and it will also fail when you try to access a feature that is undefined for the disposed object.
Conversely, if you faked the dispose because you somehow knew mA still wanted it, you would have robbed mB of it's attempt to optimize the use of the unmanaged resource.
If you absolutely need to to let the application know if the message is disposed, see
http://stackoverflow.com/questions/192206/how-does-one-tell-if-an-idisposable-object-reference-is-disposed
but it is rarely needed, otherwise it would be a commonly used pattern. The user of mA would usually just use a try catch block if he is worried about an async dispose since:
if (!mA.IsDisposed) {
// another thread calls mB.Dispose() or mA.Dispose()
mA.DoSomething();
}
still leaves a window open.
> 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.
If your class is meant to be thread safe, you have to take a deeper performance hit and acquire a lock for the duration of test and use. If you don't need thread safety, to avoid the test except on failure use:
try {
return messagep->foo();
}
catch (const std::exception& error) {
if (someDisposedTestHere) {
throw gcnew ObjectDisposedException("Message foo");
else
throw gcnew MyException(gcnew String(error.what()));
}
I believe this should convert the null pointer (unmaged space) exception for the user into the expected managed space exception.
> .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
>
> 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.
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
[jira] [Commented] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13171775#comment-13171775 ]
jiraposter@reviews.apache.org commented on QPID-3193:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3239/#review3966
-----------------------------------------------------------
trunk/qpid/cpp/bindings/qpid/dotnet/src/BindingCommon.h
<https://reviews.apache.org/r/3239/#comment8974>
I think locking inside this macro introduces hidden beviour that needs to be explicit.
Particularly as the lock scope will last (much) longer than the macro.
So I'd suggest just doing the lock explicitly in the line before the macro.
At the _very least_ rename the macro to something that indicates it locks this.
LOCK_THIS_AND_THROW_IF_DISPOSED comes to mind
- Andrew
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
[jira] [Commented] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
Posted by "Chuck Rolke (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13017613#comment-13017613 ]
Chuck Rolke commented on QPID-3193:
-----------------------------------
You are correct about the IsDisposed leaving a window open. The binding objects really want to be multithread safe so they must then do more heavy lifting.
The stackoverflow article talks about controls that have an IsDisposed property. As is, the classes don't have such a property and this Jira is a proposal to add one. But by making each class more thread safe with a lock and adding the dispose check then users won't need an IsDisposed property.
{code:title=Existing Code|borderStyle=solid}
System::String ^ Message::GetContent()
{
System::String ^ result = nullptr;
System::Exception ^ newException = nullptr;
try
{
result = gcnew String(messagep->getContent().c_str());
}
catch (const ::qpid::types::Exception & error)
{
String ^ errmsg = gcnew String(error.what());
newException = gcnew QpidException(errmsg);
}
if (newException != nullptr)
{
throw newException;
}
return result;
}
{code}
When a user generates the AccVio C# code sees the following exception. Curiously, this exception is not caught and rethrown by the GetContent function. It is caught directly by the C# caller program (which I had trouble catching previously).
{noformat}
Exception System.AccessViolationException:
Attempted to read or write protected memory.
This is often an indication that other memory is corrupt.
at qpid.messaging.Message.getContentSize(Message* )
at Org.Apache.Qpid.Messaging.Message.get_ContentSize() in
d:\qpid\cpp\bindings\qpid\dotnet\src\message.h:line 350
at Org.Apache.Qpid.Messaging.Program.Main(String[] args) in
D:\qpid\cpp\...\csharp.example.helloworld.cs:line 80.
{noformat}
The code could be fixed up by adding a lock for thread safety and a check for disposed objects that throws ObjectDisposed when seen.
{code:title=ProposedCode|borderStyle=solid}
System::String ^ Message::GetContent()
{
System::String ^ result = nullptr;
System::Exception ^ newException = nullptr;
msclr::lock lk(this);
if (NULL == messagep)
{
newException = gcnew
ObjectDisposedException("Message");
}
else
{
try
{
result = gcnew String(messagep->getContent().c_str());
}
catch (const ::qpid::types::Exception & error)
{
String ^ errmsg = gcnew String(error.what());
newException = gcnew QpidException(errmsg);
}
}
if (newException != nullptr)
{
throw newException;
}
return result;
}
{code}
This same pattern must be applied to all function and property references to all Messaging bound classes.
> .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
>
> 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.
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
[jira] [Commented] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13172659#comment-13172659 ]
jiraposter@reviews.apache.org commented on QPID-3193:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3239/
-----------------------------------------------------------
(Updated 2011-12-19 22:00:08.704694)
Review request for qpid, Andrew Stitcher, Gordon Sim, Ted Ross, Steve Huston, and Cliff Jansen.
Changes
-------
Incorporating review comments:
1. Use private lock object and not 'this'. Use same granularity as 'this' - one lock object per class object.
2. Create a ThrowIfDisposed function so same code is not replicated 40+ times.
3. Any call to destructor or to finalizer deletes unmanaged objects and effectively disposes of class object. The lock object is not deleted except by garbage collection.
4. Code is left in macro file to aid maintainability.
Summary
-------
QPID-3193 .NET Binding - proper handling of disposed objects.
This addresses bug QPID-3193.
https://issues.apache.org/jira/browse/QPID-3193
Diffs (updated)
-----
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
> .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
[jira] [Updated] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
Posted by "Chuck Rolke (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Chuck Rolke updated QPID-3193:
------------------------------
Attachment: QPID-3694_lock-and-throw-preview.patch
This patch, soon to be on Review Board, is what I suggest makes sense for a single class in the .NET Binding. It fixes the 'Message' class as that has most of the variations. The highlights are:
1. A common file with a macro in it. This macro blocks entry into disposed objects.
2. An IsDisposed property to tell if your underlying object is still there. Consider it a diagnostic.
3. A mess of THROW_IF_DISPOSED calls to block access to disposed objects.
4. Some changes to object destructor and finalizer. Destructors called (C# Dispose, C++ delete) deliberately suppress GC finalization.
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'.
This patch and the Review Board show one class. The intent is to do the same to all the binding classes.
> .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
[jira] [Commented] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13173411#comment-13173411 ]
jiraposter@reviews.apache.org commented on QPID-3193:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3239/#review4020
-----------------------------------------------------------
Ship it!
Nice work.
- Cliff
On 2011-12-19 22:00:08, 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-19 22:00:08)
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
[jira] [Commented] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
Posted by "Chuck Rolke (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13171219#comment-13171219 ]
Chuck Rolke commented on QPID-3193:
-----------------------------------
Please see https://cwiki.apache.org/qpid/c-messaging-client-net-binding-design-patterns.data/Dotnet-Binding-for-CPP-Messaging-DesignPatterns-1_2.odt for a picture of how I think the binding is *supposed* to work.
> .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
[jira] [Commented] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13171644#comment-13171644 ]
jiraposter@reviews.apache.org commented on QPID-3193:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3239/#review3964
-----------------------------------------------------------
Oops. Get rid of the braces in THROW_IF_DISPOSED. Effectively the lock goes out of scope before the object protected by the lock is referenced.
- Chug
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
[jira] [Commented] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
Posted by "Cliff Jansen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13017723#comment-13017723 ]
Cliff Jansen commented on QPID-3193:
------------------------------------
The proposed changes look very good.
As a very minor quibble, it is considered poor form to lock on "this" or any public object. I know Microsoft's own documentation is inconsistent about this, but the .NET police will usually recommend the following:
{quote}
In general, avoid locking on a public type, or instances beyond your code's control. The common constructs lock (this), lock (typeof (MyType)), and lock ("myLock") violate this guideline:
lock (this) is a problem if the instance can be accessed publicly.
lock (typeof (MyType)) is a problem if MyType is publicly accessible.
lock("myLock") is a problem because any other code in the process using the same string, will share the same lock.
Best practice is to define a private object to lock on, or a private static object variable to protect data common to all instances.
{quote}
> .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
>
> 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.
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
[jira] [Commented] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@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
[jira] [Commented] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13173309#comment-13173309 ]
jiraposter@reviews.apache.org commented on QPID-3193:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3239/#review4016
-----------------------------------------------------------
Ship it!
Looks good to me.
- Andrew
On 2011-12-19 22:00:08, 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-19 22:00:08)
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
[jira] [Resolved] (QPID-3193) .NET Binding for Messaging classes
need a test to check that binding is still in effect
Posted by "Chuck Rolke (Resolved) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Chuck Rolke resolved QPID-3193.
-------------------------------
Resolution: Fixed
Fix Version/s: 0.15
Fixed with r1222343
> .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
> Fix For: 0.15
>
> 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