Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 5,954 | Comments: 44,412

filter by tags archive

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

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

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

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

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

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

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

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

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

Peter,

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

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

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

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

Gerke

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

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

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

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

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.

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

  1. What is new in RavenDB 3.5–Intro - 15 hours from now

There are posts all the way to Jul 08, 2015

RECENT SERIES

  1. The RavenDB Comic Strip (3):
    28 May 2015 - Part III – High availability & sleeping soundly
  2. Special Offer (2):
    27 May 2015 - 29% discount for all our products
  3. RavenDB Sharding (3):
    22 May 2015 - Adding a new shard to an existing cluster, splitting the shard
  4. Challenge (45):
    28 Apr 2015 - What is the meaning of this change?
  5. Interview question (2):
    30 Mar 2015 - fix the index
View all series

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats