Ayende @ Rahien

It's a girl

Find the bug

Can you find the bug in here?

public void Receiver(object ignored)
{
	while (keepRunning)
	{
		using (var tx = new TransactionScope())
		{
			Message msg;
			try
			{
				msg = receiver.Receive("uno", null, new TimeSpan(0, 0, 10));
			}
			catch (TimeoutException)
			{
				continue;
			}
			catch(ObjectDisposedException)
			{
				continue;
			}
			lock (msgs)
			{
				msgs.Add(Encoding.ASCII.GetString(msg.Data));
				Console.WriteLine(msgs.Count);
			}
			tx.Complete();
		}
	}
}

And:

[Fact]
public void ShouldOnlyGetTwoItems()
{
	ThreadPool.QueueUserWorkItem(Receiver);

	Sender(4);

	Sender(5);

	while(true)
	{
		lock (msgs)
		{
			if (msgs.Count>1)
				break;
		}
		Thread.Sleep(100);
	}
	Thread.Sleep(2000);//let it try to do something in addition to that
	receiver.Dispose();
	keepRunning = false;

	Assert.Equal(2, msgs.Count);
	Assert.Equal("Message 4", msgs[0]);
	Assert.Equal("Message 5", msgs[1]);
}

I will hint that you cannot make any part of the receiver after it was disposed.

Comments

Peter Morris
04/24/2009 09:07 AM by
Peter Morris

My thoughts are

Why does the test have a "keepRunning" member? Surely receiver.Dispose should set keepRunning = false and then wait for the while loop to end? Personally I'd also change KeepRunning to IsDisposed or something.

But what I'd like to say most of all is this. You don't REALLY have code like Thread.Sleep(2000) in your tests do you?

Ayende Rahien
04/24/2009 09:10 AM by
Ayende Rahien

Peter,

because Receiver is the component under test, it doesn't know whatever it should keep running or not.

As for Thread.Sleep, yes, it is in the test. This is a time sensitive test.

Peter Morris
04/24/2009 09:30 AM by
Peter Morris

Right, so all of this code is in the tests then?

As for the Thread.Sleep, that's just awful :-)

Peter Morris
04/24/2009 09:36 AM by
Peter Morris

Oh, and the bug is this...

receiver.Dispose();

keepRunning = false;

Which is effectively (sometimes) this

receiver.Dispose();

msg = receiver.Receive("uno", null, new TimeSpan(0, 0, 10));

keepRunning = false;

Sergey
04/24/2009 09:52 AM by
Sergey

Probably, after while(true) loop breaks after msgs.Count == 2 receiver will be waiting for third message for 10 secs but you are disposing it in 2 secs – who is going to throw TimeoutException then?

It looks the queue is transactional. How are you going to reach msgs.Count == 2 to break while(true) loop if queue receives fail and roll-backs with moving messages to poison-message queue?

Jason
04/24/2009 11:10 AM by
Jason

receiver.Dispose will cause receiver.Receive to throw ObjectDisposedException (presumably), but then since keepRunning is still true, Receiver will call receiver.Receive again, which is the bug?

Ayende Rahien
04/24/2009 12:04 PM by
Ayende Rahien

Jason & Peter,

It will only cause it to go into a loop of throwing ObjectDisposedException, nothing that would actually cause harm, until it fall out of the loop when we set keepRunning to false.

You aren't thinking about it deeply enough, look what else may be involved.

Ayende Rahien
04/24/2009 12:06 PM by
Ayende Rahien

Peter,

Find me another wait to wait until a message does not arrive, I'll fix this

Ayende Rahien
04/24/2009 12:08 PM by
Ayende Rahien

Sergey,

When you dispose, you also break all receives, that is not actually a problem.

You are close when you are thinking about transactions, but you are going in the wrong way.

If queue receive fail, it is either a timeout or dispose, all others would cause it to exit fully

Ayende Rahien
04/24/2009 12:10 PM by
Ayende Rahien

Tommaso,

No, it can't, because that is what the test is testing

Well, not really, the test isn't testing that

It cannot be 5 because messages are received in send order, so it would always be 4

Peter Morris
04/24/2009 12:14 PM by
Peter Morris

Ayende, explain the requirement to me and I would be happy to.

Gerke
04/24/2009 12:17 PM by
Gerke

What does the transaction scope do with receiver on disposal of the scope (leave of using block) after ObjectDisposedException of receiver?

Ayende Rahien
04/24/2009 12:21 PM by
Ayende Rahien

Gerke,

DING DING DING DING

You GOT it.

And it took you a LOT less than what it took me.

For fun & games, this also can happen on another thread, isn't this FUN?

Peter Morris
04/24/2009 12:33 PM by
Peter Morris

That's the source (which I would need), but what exactly is the nature of the test?

Should get two items when two items are sent

there should be no more than two items

Ayende Rahien
04/24/2009 05:17 PM by
Ayende Rahien

Peter,

This Peter,

That test is there to uncover a bug where we would send several copies of the message to the receiver

Peter Morris
04/24/2009 08:23 PM by
Peter Morris

Ayende, I don't doubt it at all, but there are many reasons why you shouldn't be testing for that scenario in the way that you are. If the code is structured properly (which I assume it is, I haven't looked) then there are much better ways of testing for it.

Comments have been closed on this topic.