views:

206

answers:

5

OK I have a static class that has two static members, a string and a boolean.
A public static method assigns values to these members based upon the state of parameters passed in.

A private static method is then called that processes the static members.

The problem is that while the boolean keeps the value it is set to in the public function, the string does not; it defaults back to its initialised value. Why is this?
Simplified code is below.

static class MessageHandler
{
    private static String m_messageToSend = String.Empty;
    private static bool m_requiresACK = false;


    public static void Send(String message)
    {
         //formatting etc (actual method sets more fields)
         m_messageToSend = message;
         m_requiresACK = true;

         Send();
    }

    private void static Send()
    {
        SendMessageDelegate sendDelegate = DoSend;
        //At this point m_requiresACK remains true but m_messageToSend does not 
        //hold value of message; it is empty.
        IAsyncResult ar = sendDelegate.BeginInvoke(m_messageToSend, m_requiresACK);


        //rest of function
    }
}

//some other class
MessageHandler.Send("Hello");
+2  A: 

This is most likely due to another thread calling

Message.Send("");

or your AppDomain is being unloaded. Without more information it is hard to say for sure.

Andrew Hare
A: 

I would be very surprised if one field would keep the same value, and the other didn't.

If this is a web application, this can happen if the application is recycled.

Philippe Leybaert
+4  A: 

The thread "unsafetyness" of this code could be the problem, since other threads could call Send(string) while your thread is currently in the middle of the same method. I would suggest the following rewrite of the Message class:

static class Message
{
    public static void Send(String message)
    {
         Send(message, true);
    }

    private void static Send(string messageToSend, bool requiresACK)
    {
        SendMessageDelegate sendDelegate = DoSend;
        IAsyncResult ar = sendDelegate.BeginInvoke(messageToSend, requiresACK);

        //rest of function
    }
}
Peter Lillevold
Peter - this seems to have fixed the problem. As it happens I was using members directly since i've recently leaned towards this rather than passing in loads of parameters (there is actually more than listed). However of course in this example it would appear to make more sense to pass in parameters - horses for courses.
Kildareflare
Member variables are usually ok, but static member variables need special attention when it comes to thread safety. Glad it fixed the problem.
Peter Lillevold
+2  A: 

You have some huge thread safety issues there. If you really want this static, there is a cheeky fix:

[ThreadStatic]
private static String m_messageToSend = String.Empty;
[ThreadStatic]
private static bool m_requiresACK = false;

This now acts as static, but is per-thread. Crisis averted; but this is a bit... well, I would try to avoid the need, myself - but it will work fine.

important: the initializers are per thread, not per request; since it is likely that your threads will get re-used, you should be sure to initialize the state before trying to use it, or you could have old garbage.

Marc Gravell
This will only be called on one thread at a time however thanks for the tip! I did have an initialise method but removed it to reduce code.
Kildareflare
A: 

I see in your comments that after that first line, the private variable is empty instead of holding the value of the parameter. Is this true as soon as you hit the first line of Send() ?

Write a unit test or a simple test harness that calls Message.Send("Hello World"); and does an assert on the output. Isolating this from your calling codebase could shine light on whether it is your Message class that is behaving abnormally, or if it is a consumer sending bad/unexpected data.

Also, unless your //rest of function includes resetting your bool, it will always be true after the first message is sent.

Andy_Vulhop
As it happens yes the bool is reset - there is a lot of other formating and logic too. I simplified the code a lot so that it focused on just the issue I was having problems with. I think in future I will add more comments to highlight this...well spotted though.
Kildareflare
Understood, and kinda expected. I just added that as an off chance that it was not being reset. Every once in a while the low hanging fruit actually works out.
Andy_Vulhop