Xml messages processing networkstream [closed]

Question

I'm currently processing xml messages with C#. It is working But I'm not confident that my code is fast enough. There are 3 possible messages I can receive. When I receive one the message an event is triggered and I can access the object outside of this thread and do the necessary processing. The method start is running in a separate thread.

    public void Start()
    {
        NameTable nt = new NameTable();
        object frameAck = nt.Add("frameack");
        object alarmResponse = nt.Add("alarmresponse");
        object watchdog = nt.Add("watchdog");
        StreamReader sr = new StreamReader(_tcpClient.GetStream());
        XmlReaderSettings settings = new XmlReaderSettings
        {
            NameTable = nt,
            ConformanceLevel = ConformanceLevel.Fragment
        };
        XmlReader xr = XmlReader.Create(sr, settings);
        while (!_requestStop)
        {
            try
            {
                while (xr.Read())
                {
                    if (xr.NodeType == XmlNodeType.Element)
                    {
                        object localName = xr.LocalName; // Cache the local name to prevent multiple calls to the LocalName property.
                        if (watchdog == localName)
                            OnWatchdogComplete(new CustomEventArgs(xr.ToClassFromXmlStream<WatchdogFrame>()));
                        if (alarmResponse == localName)
                            OnAlarmResponseComplete(new CustomEventArgs(xr.ToClassFromXmlStream<AlarmResponse>()));
                        if (frameAck == localName)
                            OnAckComplete(new CustomEventArgs(xr.ToClassFromXmlStream<FrameAck>()));
                    }
                }
            }
            catch (Exception e)
            {
                Console.WriteLine(e.Message);
            }
        }
        xr.Close();
        sr.Close();
        Console.WriteLine("Stopping TcpXmlReader thread!");
    }

This is my extensionmethod to deserialize to an object.

    public static T ToClassFromXmlStream<T>(this XmlReader xmlStream)
    {
            XmlSerializer serializer = new XmlSerializer(typeof(T));
            return (T)serializer.Deserialize(xmlStream);
    }

When I close the program I always get the errors:

There is an error in XML document (9, 12).

Cannot access a disposed object.

Object name: 'System.Net.Sockets.NetworkStream'.

When I was string processing this it would seem that is was going a lot faster. But it was very complex and this very easy.

Answer 1

I think this question is half-way between Stack Overflow and Code Review anyway...

Start() is running on a standalone thread however it uses _tcpClient. Assuming things are properly done you dispose _tcpClient in container class Dispose() method. When it happens your reader will access (indirectly, through sr of type StreamReader) a disposed object (_tcpClient, from which underlying stream comes from).

You may transfer ownership of _tcpClient to your secondary thread.

void Start() {
    try {
        // All the code you already have in-place
    }
    finally {
        _tcpClient.Dispose();
    }
}

However you already have a flag in-place (_requestStop). Hard to say (without surrounding code) what's wrong but you first should check if you set flag _requestStop and immediately exit (even if reading is in progress). You'd better set that flag and then wait until thread completes its work (for an example see How to wait for thread to finish with .NET?).

---

Few more considerations more on-topic on Code Review.

1) You're not using using statement:

using (var sr = new StreamReader(_tcpClient.GetStream())) {
    // You don't need to explicitly call sr.Close();
}

Do the same for your XmlReader variable xr.

2) XML deserialization isn't as quick as you may wish. If speed is a measured issue then you may consider to manually parse XML fragment (using XmlReader you have) to build your objects.

3) You should avoid to catch a generic Exception. You should know what may go wrong in your code, catch errors you expect and leave to a more generic error handler to catch the others (for example, no matters how many times you retry because of while() but if _tcpClient has been closed then it won't recover its connection and you'll keep generating exceptions).

4) In your big try/catch you also catch exceptions generated by code called in events. Also some exceptions may leave XmlReader in an inconsistent state and you may want to re-create it.

5) You do not need to cache XmlReader.LocalName property value. It won't perform any parsing for each access and call to property getter may be optimized away (inlined) by JIT compiler. It's such micro-optimization that you won't notice any difference even if you read its value Int32.MaxValue times (do not forget you're also working with network data).

6) XmlReader.LocalName type is string, no reason to loose its type assigning to an object variable. The same is true for NameTable.Add() return type: it's a string, don't assign it to an untyped object variable.

7) You compare strings (localName with watchdog and the others) but you do not explicitly say which type of comparison you want. Is it case-sensitive or case-insensitive? Is current culture aware? Make it explicit according to what you want or you may have surprises when current culture has unexpected text comparison rules (see also this post). Change your code to:

if (String.Equals(xr.LocalName, watchdog, StringComparison.InvariantCulture)) {
}

Answer 2

I can't speak to the performance, but you should make sure your resources get closed properly when an exception occurs. Either switch to a using statement, or move your clean up code to a finally block.