[rabbitmq-discuss] .NET client SimpleRpcServer question

Ryan,

Ryan Davis wrote:
Looking at the code, I am pretty sure that Close() is meant to be called
from inside the HandleCall/Cast functions. Calling it from anywhere else
is probably unsafe, and the error you are getting looks
threading-related. It would be useful to get a stack trace with line
numbers to work out exactly where the NPE occurs.


Regards,

Matthias.

Really? The code comment on Close() says "Shut down the server, causing
MainLoop() to return to its caller.", which is exactly what I want. I
dug deeper into SimpleRpcServer, and what I really need is to break from
MainLoop's foreach after the body of the loop has executed. The
m_subscription is used after HandleCall / HandleCast / ProcessRequest,
and I don't see how I could close it in HandleCall without creating
problems later on down the line.

If I call Close() at any point in HandleCall/Cast, then the byte[]
intended for the RPC caller will be lost, and I expect it would throw a
nullref when it tries to BasicPublish those bytes.
If I call Close() at the end of ProcessRequest, then the message will be
processed and the ack will be lost. This isn't so bad as I have other
logic checking for duplicates, but I expect it would throw a nullref
when it tries to ack in MainLoop.
If I call Close() at the beginning of ProcessRequest, then the message
will not be processed and the ack will be lost. This isn't so bad as we
should have a redelivery, but I expect it would throw a nullref when it
tries to ack in MainLoop.
I'm afraid I might be a minor version behind at this point.
System.NullReferenceException: Object reference not set to an instance of an object.
at RabbitMQ.Client.Impl.ModelBase.BasicCancel(String consumerTag) in d:\Projects\rabbitmq-dotnet-client-1.5.3\src\client\impl\ModelBase.cs:line 668
at RabbitMQ.Client.MessagePatterns.Subscription.Close() in d:\Projects\rabbitmq-dotnet-client-1.5.3\src\client\messagepatterns\Subscription.cs:line 223
at RabbitMQ.Client.MessagePatterns.Subscription.System.IDisposable.Dispose() in d:\Projects\rabbitmq-dotnet-client-1.5.3\src\client\messagepatterns\Subscription.cs:line 454
at RabbitMQ.Client.MessagePatterns.SimpleRpcServer.MainLoop() in d:\Projects\rabbitmq-dotnet-client-1.5.3\src\client\messagepatterns\SimpleRpcServer.cs:line 205

Line 668 is:
ModelShutdown -= new
ModelShutdownEventHandler(k.m_consumer.HandleModelShutdown);

I think I'm calling subscription.Close() multiple times, since my code
is essentially:

using(var subscription = new Subscription(model, "queue", false))
using (RpcServer = new MyRpcServer(subscription, this)) {
RpcServer.MainLoop();
}

So I think I'm calling subscription.Close() 3 times in 2 threads:
# controller thread: calls RpcServer.Close()
# worker thread: when the using() invokes subscription.Dispose()
# worker thread: when the using() invokes RpcServer.Dispose()

Subscription.Close() is checking if m_consumer is null before calling
BasicCancel, so it looks like a race condition between the controlling
thread calling RpcServer.Close() and the worker thread cleaning up with
Dispose(), but I don't follow how the foreach in MainLoop would
terminate and start Disposing of things before the controller thread had
gotten through subscription.Close() to set m_consumer to null.

I've got a unit test that reproduces this behavior, so I'll be playing
with this today, stepping through to better understand what is happening.

As a side note, the docs on
http://www.rabbitmq.com/build-dotnet-client.html could use mention of
the local.build file.

Thanks,

Ryan Davis
Acceleration.net
Director of Programming Services
2831 NW 41st street, suite B
Gainesville, FL 32606

Office: 352-335-6500 x 124
Fax: 352-335-6506

I think I figured it out. The C# "foreach" loop also acts like a
"using" statement, and will call Dispose() on my subscription when it
finishes, which I didn't realize
(http://mark.michaelis.net/Blog/CForeachWithArrays.aspx). This means
I'm actually calling Subscription.Close() a total of 4 times.

Here's the chronology
# controller thread: calls RpcServer.Close() -> Subscription.Close() ->
Model.BasicCancel()
# worker thread: throws and catches exceptions in Subscription.Next(),
terminating MainLoop's foreach
# worker thread: calls Subscription.Dispose() -> Subscription.Close() ->
Model.BasicCancel()
# controller thread: completes Model.BasicCancel(), sets some variables
to null
# worker thread: nullref trying to use those variables

I added some Console.WriteLine statements to the Close/Dispose methods,
and that solved the problem, so it's definitely a race condition when
calling Close() from another thread.

I'm going to add a flag and test out calling Close() from various points
in HandleCall/HandleCast/ProcessRequest.

Thanks,

Ryan Davis
Acceleration.net
Director of Programming Services
2831 NW 41st street, suite B
Gainesville, FL 32606

Office: 352-335-6500 x 124
Fax: 352-335-6506

Wow, I really didn't think that one through.
HandleCall/HandleCast/ProcessRequest are only called when there is a
message to process. If there is no message to process, the
SimpleRpcServer is blocked on Subscription.Next(), and will never
exit. My current test case starts a SimpleRpcServer, then tries to
stop it, without sending any messages.

I changed my code so if the SimpleRpcServer sees a certain byte[]
message (stored as ShutdownKey), it will close itself:

public override void HandleCast(bool isRedelivered, IBasicProperties
requestProperties, byte[] body)
{
for (var i = 0; i < body.Length; i++) {
if (body[i] != ShutdownKey[i]) return;
}
Close(); //we must have matched, shut down.
}

As I expected, this throws a different exception:
System.InvalidOperationException: Operation is not valid due to the current state of the object.
at RabbitMQ.Client.MessagePatterns.Subscription.Next() in d:\Projects\rabbitmq-dotnet-client-1.5.3\src\client\messagepatterns\Subscription.cs:line 323
at RabbitMQ.Client.MessagePatterns.Subscription.System.Collections.IEnumerator.MoveNext() in d:\Projects\rabbitmq-dotnet-client-1.5.3\src\client\messagepatterns\Subscription.cs:line 437
at RabbitMQ.Client.MessagePatterns.SimpleRpcServer.MainLoop() in d:\Projects\rabbitmq-dotnet-client-1.5.3\src\client\messagepatterns\SimpleRpcServer.cs:line 206

I'm going to start mucking about in SimpleRpcServer.cs/Subscription.cs
to see what's the smalled change I can make to fix it.

Thanks,

Ryan Davis
Acceleration.net
Director of Programming Services
2831 NW 41st street, suite B
Gainesville, FL 32606

Office: 352-335-6500 x 124
Fax: 352-335-6506

Ok, last one today, I promise.

I solved this problem my adding some explicit locks in Subscription and
SimpleRpcServer. I tried to checkout the latest source (on linux,
cygwin, and tortoiseHG) but kept getting a zlib error, so this patch is
against the source in rabbitmq-dotnet-client-1.5.3.zip downloaded from
http://www.rabbitmq.com/dotnet.html.

This patch makes SimpleRpcServer.Close threadsafe by adding:

1. locking SimpleRpcServer.m_subscription around the body of
SimpleRpcServer.MainLoop's foreach (so we can't close while
processing)
2. checking for a null Subscription.Consumer before processing a
message in SimpleRpcServer.MainLoop, but after we've locked
m_subscription (to ensure we don't process after we've closed)
3. locking this around the body of Subscription.Close (to ensure we
don't close that same subscription at the same time)

I *think* that solves the potential race conditions. It certainly makes
my tests work, and running "nant unit" on the 1.5.3 source gives 2
failures in TestSharedQueue.cs that look timing-related.

Thanks,

Ryan Davis
Acceleration.net
Director of Programming Services
2831 NW 41st street, suite B
Gainesville, FL 32606

Office: 352-335-6500 x 124
Fax: 352-335-6506



Ryan Davis wrote:

Ryan,

Ryan Davis wrote:
Thanks for the patch. If you don't mind, we'll look into this in a few
weeks when the person who wrote the original code is back from holidays.

Some locking is most certainly required if you are going to call Close
from a separate thread. So the question is whether we should cater for
this in the first place (based on your findings I am pretty sure we need
to), and if so whether the locks are in the right place.

On the latter point, the locks look a bit too coarse-grained to me,
covering activities that can take an indeterminate amount of time, such
as interactions with the server. That may be unavoidable, but without
understanding every single detail of the code it's hard to be sure.


Regards,

Matthias.

Certainly. I'll put my patch into production today, and will send you a
new patch if I run into problems.
They are a bit heavy-handed. It is very possible that Subscription /
SimpleRpcServer could be refactored to maintain thread safety with finer
locks. I was wanting to make minimal changes, so I didn't go that
route. I walked through it with another co-worker for a sanity check,
and locking those large blocks seemed the only way to ensure correctness
in these cases:

1. calling Close() while the subscription is blocked waiting for a
message
2. calling Close() while the server is processing a message
3. calling Close() after the subscription has fetched a message but
before the server has started processing it

I'm looking forward to hearing from the original author next month.

Thanks,

Ryan Davis
Acceleration.net
Director of Programming Services
2831 NW 41st street, suite B
Gainesville, FL 32606

Office: 352-335-6500 x 124
Fax: 352-335-6506

Has your .NET programmer had a chance to look at this patch yet? I have
not had any problems running it in production (albeit with a light
message load).

Thanks,

Ryan Davis
Acceleration.net
Director of Programming Services
2831 NW 41st street, suite B
Gainesville, FL 32606

Office: 352-335-6500 x 124
Fax: 352-335-6506

Hi Ryan,

Ryan Davis wrote:
I've taken a look at the issue this morning; things are moving at last.
Sorry for the delay. I've just committed a proposed fix with a lighter
locking pattern to branch bug20945; hopefully it will pass QA soon and
be merged into default.

Regards,
Tony

Great! No worries on the delay, hope you had a great holiday. I'd like
to clarify where I'm supposed to call SimpleRpcServer.Close(). Looking
at the Shutdownable examples in branch bug20945, it looks like I'm
supposed to send the server a special "shut me down" AMQP message, and
have the server call Close(). Is that correct?

Thanks,

Ryan Davis
Acceleration.net
Director of Programming Services
2831 NW 41st street, suite B
Gainesville, FL 32606

Office: 352-335-6500 x 124
Fax: 352-335-6506

Hi Ryan,

Ryan Davis wrote:
The example has two styles of call to Close: one from within the
server's own mainloop, as you describe, but also one in a detached
thread sitting outside the mainloop. It seems important to me to be able
to robustly shut down a Subscription from both within and without. So
your original example should work fine, too.

Tony