+972 52-548-6969
Find the bug
time to read 1 min | 120 words
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
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?
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.
Right, so all of this code is in the tests then?
As for the Thread.Sleep, that's just awful :-)
msgs[0] could be "Message 5"
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;
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?
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?
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.
Peter,
Find me another wait to wait until a message does not arrive, I'll fix this
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
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
Ayende, explain the requirement to me and I would be happy to.
rhino-tools.svn.sourceforge.net/.../queues
What does the transaction scope do with receiver on disposal of the scope (leave of using block) after ObjectDisposedException of receiver?
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?
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
Peter,
This Peter,
That test is there to uncover a bug where we would send several copies of the message to the receiver
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