2

I was recently asked a thread safety question in a c# interview. I didn't get it quite right. I'm am trying to understand it now. Here's the question...

Given a basic Order class...

using System;

namespace OrderThreadTest
{
    public class Order
    {
        public TimeSpan CancelTime;
        public int Id;
    }
}

Given a simple schedule class stub however I have implemented it to test at home...

using System;
using System.Threading;

namespace OrderThreadTest
{
    public class Scheduler : IDisposable
    {
        private Timer _timer;

        public IDisposable Schedule(Order order, Action action)
        {
            var current = DateTime.Now;
            var timeToGo = order.CancelTime - current.TimeOfDay;

            _timer = new Timer(x => { action(); }, null, timeToGo, Timeout.InfiniteTimeSpan);

            return this;
        }

        public void Dispose()
        {
        }
    }
}

How do you ensure that the Cancel method in the worker class is thread safe? My answer is the commented out pieces

using System;

namespace OrderThreadTest
{
    public class Answer
    {
        private readonly Scheduler _scheduler;
        private object _myLock = new object();
        private Order _order;

        public Answer(Scheduler scheduler)
        {
            _scheduler = scheduler;
        }

        public void Cancel(Order order)
        {
            //  lock (_myLock)
            //  {
            // _order = order;
            //  var result =
            _scheduler.Schedule(order, () =>
            {
                //if (order.Equals(_order))
                //{
                Console.WriteLine("Canceled: " + order.Id);
                order = null;
                //}
            });
            //  }
        }
    }
}

My first question in understanding this is how can I cause an example of a second thread setting the passed in Order and changing a earlier timer? For example cause a thread data clash.

I have tried like this but it always seems to run as expected...

using System;

namespace OrderThreadTest
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            var a1 = new Answer(new Scheduler());

            var o = new Order
                {Id = 1, CancelTime = new TimeSpan(DateTime.Now.Hour, DateTime.Now.Minute, DateTime.Now.Second + 5)};

            a1.Cancel(o);

            a1.Cancel(o);

            Console.ReadLine();
        }
    }
}

How do I repro the problem that I am meant to solve here?

Ben Cameron
  • 4,335
  • 6
  • 51
  • 77
  • I didn't actually get feedback on my solution. It was all done on paper. Now I;m trying it on a PC at home. – Ben Cameron Jan 23 '19 at 21:36
  • You're not scheduling your order. Each call to Cancel you're creating a new Order, so they're inherently threadsafe because they're separate objects. For this to be a meaningful test you need to create a new order, schedule it, then cancel it twice, and handle that cancellation in a threadsafe manner (i.e., handle the fact it may already be canceled, or in the process of being cancelled). – Ben Jan 23 '19 at 21:40
  • `_timer = new Timer(x => { action(); }, null, timeToGo, Timeout.InfiniteTimeSpan);` What happens if two threads execute that line in quick succession? Does perhaps one of the timers not fire? How would that differ if your `_timer` variable contained a list / set of timers? – mjwills Jan 23 '19 at 21:41
  • @Ben - I have edited the Program. Is this what you mean? – Ben Cameron Jan 23 '19 at 21:46
  • Oh, the Cancel method calls schedule? So you're scheduling a cancellation? Or is what you've done so far wrong? Either way, the fact you're operating on separate Order means you won't have any clashes. – Ben Jan 23 '19 at 21:46
  • The cancel sets up the sheduler to do the cancel. – Ben Cameron Jan 23 '19 at 21:48
  • @Ben The fact there is only a single `Timer` variable suggests there may be an ability for a later `Schedule` call to effectively disable an earlier `Schedule` call. – mjwills Jan 23 '19 at 21:48
  • 1
    Possible duplicate of [Best way to create a "run-once" time delayed function in C#](https://stackoverflow.com/questions/5904636/best-way-to-create-a-run-once-time-delayed-function-in-c-sharp) – mjwills Jan 23 '19 at 21:49
  • @BenCameron Yes, now you're dealing with the same object. Now you need to be able to handle that order being scheduled to be canceled twice at exactly the same time. – Ben Jan 23 '19 at 21:50
  • @Ben - sorry I don't understand what you mean? – Ben Cameron Jan 23 '19 at 21:51
  • Did they put something else that you didn't mention thread safe how? Nothing in this whole code will produce a bug if you run it in multiple threads and the _timer is useless. Was there something else in the question that you forgot to mention? – Filip Cordas Jan 23 '19 at 21:58
  • @FilipCordas What do you mean by the timer being useless? – mjwills Jan 23 '19 at 22:02
  • The only parts I was actually given were the order and answer stub. I've written the scheduler as part of soling it at home. – Ben Cameron Jan 23 '19 at 22:03
  • 1
    Remember, "it runs and passes my tests" is not strong evidence for the proposition "my code is thread safe under all possible legal combinations of processor, jitter and compiler behaviours". To know that a piece of multithreaded code is safe requires a *logical proof* that the memory model of the language *enforces* that the code meets its postconditions. – Eric Lippert Jan 23 '19 at 22:21
  • So I'm trying to take what they gave me, make it thread unsafe so then I can make it thread safe. – Ben Cameron Jan 23 '19 at 22:25
  • 1
    " I've written the scheduler as part of soling it at home" Well that makes things make more sense. Do you have the actual text of the assignment. Because I think you missed some things in the question. – Filip Cordas Jan 23 '19 at 22:36
  • One of the problems with this question is that it is completely unclear what operations are permitted. For example, the order class has public, non-volatile, writable fields and one of them is larger than int. So, are we worried about torn reads and writes to that field? Nothing prevents them. All of this code is a mass of bad practices that could lead to threading problems; it's hard to know how to make it right because we don't know what the person asking the question considers to be a possible misuse. – Eric Lippert Jan 23 '19 at 22:46

0 Answers0