2

I'm attempting to dispose of a NetworkStream after it's finished writing. I've tried wrapping the stream in a using() like so:

using (NetworkStream stream = client.GetStream())
{
     foreach (byte[] command in fadeSceneOut)
     {
          if (stream.CanWrite)
          {
               stream.BeginWrite(command, 0, command.Length, new AsyncCallback(SendCallback), stream);
          }
     }
}

But I receive a System.ObjectDisposedException stating that the object has already been disposed and cannot be accessed in my callback where I send an EndWrite():

private static void SendCallback(IAsyncResult ar)
{
    try
    {
        NetworkStream stream = (NetworkStream)ar.AsyncState;
        stream.EndWrite(ar);
    }
    catch (Exception e)
    {
        Console.WriteLine(e.ToString());
    }
}

My understanding was that the stream would not be disposed until execution leaves the using block after all commands have been written to the stream. Obviously I've gotten something wrong here, can someone advise on the correct approach?

Edit: included an await approach:

static void Main(string[] args)
{
     Task.WaitAll(Run());
}

public static async Task Run()
{
    // Get a scene
    var scene = GrabSceneFromUser();

    // Get scene commands
    var fadeSceneIn = LightSwarmHelper.BuildSceneCommands(scene);
    var fadeSceneOut = LightSwarmHelper.BuildSceneCommands(scene, false);

    // Send commands to device
    using (TcpClient client = new TcpClient(ConfigurationManager.AppSettings["Host"], Convert.ToInt32(ConfigurationManager.AppSettings["Port"])))
    {
        using (NetworkStream stream = client.GetStream())
        {
            foreach (byte[] command in fadeSceneOut)
            {
                if (stream.CanWrite)
                {
                    await stream.WriteAsync(command, 0, command.Length); //stream.BeginWrite(command, 0, command.Length, new AsyncCallback(SendCallback), stream);
                }
            }
        }
    }
}
DGibbs
  • 14,316
  • 7
  • 44
  • 83
  • This code is obsolete, use await. Next, you are creating unbounded amounts of concurrent sends. This can easily exhaust resources. Perform the sends sequentially. It might even result in corruption, not sure. – usr Dec 02 '15 at 09:55
  • @usr Not familiar with `await`. Couldn't find any examples of it with a `NetworkStream` either, can you elaborate? – DGibbs Dec 02 '15 at 10:02
  • Google for ".net NetworkStream await". – usr Dec 02 '15 at 10:03
  • @usr Thank you. Regarding `you are creating unbounded amounts of concurrent sends. This can easily exhaust resources.`, there will be no more than a 4-5 writes each time the code is called. Is this an issue? – DGibbs Dec 02 '15 at 10:07
  • That's not a perf issue but I don't know if this is even valid to do. No need, just use await and write a normal sequential loop. – usr Dec 02 '15 at 10:55

2 Answers2

3

Since the call to stream.BeginWrite(... ) is non-blocking, the using-block will be left before the write operation completes, hence the exception. The proper place to dispose of the stream would be in the callback function.

StefanFFM
  • 1,526
  • 1
  • 14
  • 25
  • This was my first guess but I still get the same error, presumably because I'm kicking off different threads for each command, it would then dispose of the stream before all threads have finished executing – DGibbs Dec 02 '15 at 09:38
  • You'll need to keep track of how many write operations have completed and then dispose the stream only if it was the callback to the last write. – StefanFFM Dec 02 '15 at 09:53
1

Stefans answer is correct and points to the root cause. You should accept it. I'll add another way to solve the problem:

This code is obsolete, use await. Next, you are creating unbounded amounts of concurrent sends. This can easily exhaust resources. It might even result in corruption, not sure.

Perform the sends sequentially. This is very easy and natural with await.

if (stream.CanWrite)

This is not thought through... CanWrite will always return true and if it returned false you simply would do nothing and hide a bug.

usr
  • 168,620
  • 35
  • 240
  • 369
  • I'm going to update my question with some sample code using `await`. It works, but it feels wrong to me. Could you take a look? – DGibbs Dec 02 '15 at 11:41
  • Looks correct. Any concrete worries? Delete the send buffer thing. – usr Dec 02 '15 at 11:46
  • Running this is a console app at the moment, it seems like it's not waiting until everythings been written to the stream as I sometimes only see one command sent, sometimes none at all. If I add a `Console.Read();` it sends them all. My goal is to have it wait until everything has been written and then dispose of the stream – DGibbs Dec 02 '15 at 11:53
  • Common mistake, you need to wait for the operations to complete in Main. Calling Wait() does the job. This actually looks like you should not be using async IO at all. Make yourself familiar with why and how it can be helpful. Probably, it does only damage here. – usr Dec 02 '15 at 12:13
  • I'm using `Task.WaitAll(Run());` however it still occasionally only does a couple of the stream writes. – DGibbs Dec 02 '15 at 12:16
  • Oh, you are not disposing of the TcpClient. Should have seen that immediately. – usr Dec 02 '15 at 12:17
  • Have edited. Would I make the async task return `Task` and then get this from `task.Result` to dispose of it? – DGibbs Dec 02 '15 at 12:19
  • Just put client and stream in using blocks. Nothing complicated. it also is a good idea to call Socket.Shutdown(Both). This makes sure errors are noticed. – usr Dec 02 '15 at 12:23
  • I only get one write to the stream if I do this – DGibbs Dec 02 '15 at 12:26
  • Post that code. You need to do that in any case even if it does not solve the immediate issue. – usr Dec 02 '15 at 12:27
  • Hm, this should work. Did you write the receiving program as well? Maybe its broken. – usr Dec 02 '15 at 12:36
  • Are the commands known to be correct? The TCP code looks really solid now. I don't know how command could be lost here. Maybe do some print debugging and print the buffers to the console right before sending. – usr Dec 02 '15 at 12:41
  • They provide a a tool for calculating the hex of commands, I've compared mine to the generated and they are identical. Doesn't seem to be a problem with the commands – DGibbs Dec 02 '15 at 12:43
  • I give up, then :) If I had to bet you will be discovering that you are not calling WriteAsync with the data you expect. Maybe hard code all the buffers into the C# source to be sure. Also delete that CanWrite check. – usr Dec 02 '15 at 12:44
  • Okay, thanks for your help :) I've got @Stefans answer working, perhaps I'll revert to this. – DGibbs Dec 02 '15 at 12:48
  • Well, it's probably not working because of usage of APM. APM and TAP are equivalent, TAP is a thing wrapper around APM. – usr Dec 02 '15 at 13:58