Properly disposing a WebRequest and StreamReader

Question

I'm getting an object disposed exception during a call to the ReadToEnd method from a client side method, getRecords, that communicates to a webserver using a StreamReader.

The first call to getRecords succeeds, it is only during a subsequent call the exception occurs, and so I'm not closing and disposing of the StreamReader and associated WebRequest properly.

I'm aware that I could wrap these two objects in a using statement, however that just gets expanded into a try/catch/finally statement. As can be seen in my code below, I'm cleaning up in my finally clause.

Therefore, I'm either not doing something that the using statment does, or there is something else I may be missing in my finally statment. I'd rather not using the using statment if at all possible, as I like my code explicit.

Here is the code and the associated exception:

    public int getRecords(string[] args, string[] vals)
    {
        List<string> urlList = BuildUrlRequestStrings(args, vals); 

        WebRequest request = null;
        WebResponse wresponse = null;
        StreamReader sr = null;           

        foreach (string url in urlList)
        {   
            request = WebRequest.Create(url);

            request.Method = "GET";
            request.ContentType = "application/json";
            //request.Timeout = -1;
            request.Timeout = 300000;
            request.Credentials = CredentialCache.DefaultCredentials;
            //request.ContentType = "application/xml";

            try
            {
                wresponse = request.GetResponse();

                /*using (StreamReader sr = new StreamReader(wresponse.GetResponseStream()))
                {
                    _recieveBuffer = sr.ReadToEnd().ToString();
                }*/
                sr = new StreamReader(wresponse.GetResponseStream());
                _recieveBuffer = sr.ReadToEnd();

                //List<T> temp = JsonConvert.DeserializeObject<List<T>>(_recieveBuffer);
                List<T> temp = JsonConvert.DeserializeObject<List<T>>(
                    _recieveBuffer,
                    new JsonSerializerSettings { TypeNameHandling = TypeNameHandling.All }
                );

                _recieveData.AddRange(temp);                    
            }
            catch (WebException ex)
            {
                if (ex.Response != null)
                {
                    // can use ex.Response.Status, .StatusDescription         
                    if (ex.Response.ContentLength != 0)
                    {
                        using (var stream = ex.Response.GetResponseStream())
                        {
                            using (var reader = new StreamReader(stream))
                            {
                                Log.Info(FIDB.TAG1, "   WSBuffer.getRecords: WEBSERVER MESSAGE: " + reader.ReadToEnd());
                            }
                        }
                    }
                }

                return -1;
            }
            finally
            {
                if (sr != null)
                {
                    sr.Close();
                    sr.Dispose();
                }

                if (wresponse != null)
                {
                    wresponse.Close();
                    wresponse.Dispose();
                }
            }
        }

        return _recieveData.Count;
    }

---

07-02 11:32:15.076: I/<<< FI >>>(2775): StorageRelayService.RequestQueueThread: EXCEPTION: System.ObjectDisposedException: The object was used after being disposed. 07-02 11:32:15.076: I/<<< FI >>>(2775): at System.Net.WebConnection.BeginRead (System.Net.HttpWebRequest request, System.Byte[] buffer, Int32 offset, Int32 size, System.AsyncCallback cb, System.Object state) [0x00000] in :0 07-02 11:32:15.076: I/<<< FI >>>(2775): at System.Net.WebConnectionStream.BeginRead (System.Byte[] buffer, Int32 offset, Int32 size, System.AsyncCallback cb, System.Object state) [0x00000] in :0 07-02 11:32:15.076: I/<<< FI

(2775): at System.Net.WebConnectionStream.Read (System.Byte[] buffer, Int32 offset, Int32 size) [0x00000] in :0 07-02 11:32:15.076: I/<<< FI >>>(2775): at System.IO.StreamReader.ReadBuffer () [0x00000] in :0 07-02 11:32:15.076: I/<<< FI >>>(2775): at System.IO.StreamReader.Read (System.Char[] buffer, Int32 index, Int32 count) [0x00000] in :0 07-02 11:32:15.076: I/<<< FI (2775): at System.IO.StreamReader.ReadToEnd () [0x00000] in :0 07-02 11:32:15.076: I/<<< FI >>>(2775): at FieldInspection.Shared.Buffer.WSBuffer1[FieldInspection.Shared.Model.AggregateRoot.Parcel].getRecords (System.String[] args, System.String[] vals) [0x00000] in <filename unknown>:0 07-02 11:32:15.076: I/<<< FI >>>(2775): at FieldInspection.Shared.Repository.REST.RepositoryREST1[FieldInspection.Shared.Model.AggregateRoot.Parcel].Read (IConditions conditions) [0x00000] in :0 07-02 11:32:15.076: I/<<< FI >>>(2775): at FieldInspection.Shared.Model.DataAccess.ParcelRepositoryREST.parcelByIdList (System.Collections.Generic.List1 parcelIdList, Boolean bCurrent, Boolean bHistorical) [0x00000] in <filename unknown>:0 07-02 11:32:15.076: I/<<< FI >>>(2775): at FieldInspection.Droid.StorageRelayService.ProcessRequestGetParcelCache (FieldInspection.Shared.Database.IPC.Request request) [0x00000] in <filename unknown>:0 07-02 11:32:15.076: I/<<< FI >>>(2775): at FieldInspection.Droid.StorageRelayService.ProcessRequestFromForegroundActivity (System.Collections.Generic.List1 reqList) [0x00000] in :0 07-02 11:32:15.076: I/<<< FI >>>(2775): at FieldInspection.Droid.StorageRelayService.RequestQueueThread () [0x00000] in :0

Solution 1

Make use of using which will close your connection once it loses scope.

Solution 2

I'd highly suggest you to use "using" statement. Once you know your WebResponse and StreamReader disposed properly, it becomes easier to debug.

Also, you create a WebRequest object for each iteration of your loop. Why don't you try an asycnhronous approach?

This post might be helpful: How to use HttpWebRequest (.NET) asynchronously?

Solution 3

I know you already accepted an answer, but an alternative solution would be to put the variable declarations inside the foreach loop.

foreach (string url in urlList)
{ 
    WebRequest request = null;
    WebResponse wresponse = null;
    StreamReader sr = null;  
    ...
}

Then each loop will get it's own instance, and .Dispose() will work as you expect.

Comments

[Galvion]

You could clean your using statements up with the following : stackoverflow.com/questions/1329739/…

[Nathan Phetteplace]

Is there a specific reason you don't want to just go with a using statement?

[samus]

@Jeff Hubbard I don't like the "unkown" factor of when and where the finally clause and dispose statment is called when the using statment is expanded, and the foreach loop is a perfect example here. As stated, I'm not supposed to call dispose before the loop exists, b/c the object will be instantiated again, thus setting to null instead (as suggested). If you see my comment in the answer below, I ask "would the compiler know not to call dispose until the loop exited". These are the uncertainties I like to avoid.

[Bobson]

@SamusArin - If the using was inside the foreach, then the object would be garbage collected after each iteration, as if you started your foreach with StreamReader sr = null;. If the using wrapped the foreach, then you wouldn't be able to assign to it from within it, so it wouldn't need to repeatedly close. This is a perfect example of why using using is a good thing - you know exactly what scope that stream is valid for, and you never leave it in an in-between state.

[samus]

@Bobson Well ok then, I'll give the using statment a try, now that I know how it works. Sage tip, thank you.

[John Saunders]

-1: setting to null has no effect, and sometimes even a negative effect.

[samus]

Ahh, I remember running into a similar situation with my connection to a (local) sqlite db, in the same app. I had to clean up in a very specific order. This is good info, thank you. So, if a "using" statment was used within a foreach, would the compiler know not to call dispose until the loop exited (I'll decompile eventually to investigate) ?

[Aaron McIver]

Setting to null was in conjunction to calling Dispose, which should allow reuse of the object in the foreach. Sticking with using and pushing the Exception upstream is certainly preferred, or to stick with calling Close and not Dispose.

[samus]

I had to set to objects to null for my sqlite connections to clean up and not leave file pointers open: if (_rdr != null) { _rdr.Close(); _rdr = null; } if (_cmd != null) { _cmd.Dispose(); _cmd = null; } if (_con != null) { _con.Close(); _con = null; }

[samus]

Well, using hides the finally clause that the compiler ends up generating, so I don't really see how this helps in understanding how the SR and WR get disposed. Also, I stated that I don't prefer the using statment (too obscure, and "magical"). The loop is actually to facilitate a url string that exceeds the size limit, and so each request is a sub request, and so sequential is fine. I'm taking the loop out anyway, since I can pass arguments in a JSON serialized object (which hides them as well). Thanks for tip about async tho.

[samus]

@Aaron McIver "Setting to null was in conjunction to calling Dispose"... Did you mean "to calling Close" ?

[samus]

@Aaron McIver No problem, thank you. Have a good holiday, too !)

[samus]

@Aaron McIver ... actually, calling Close alone, without setting to null works (for my specific case, and only after a few tests). Nonethess, your help got it working for me, and gave me the insight I needed.

[Tequilalime]

You're welcome. I see, you may not prefer the using statement, but since it ensures your objects will be disposed when they're done, you won't get lost in try{..}catch{..}finally{...} blocks. I personally find it easier to use.

[samus]

Heh, you may not believe me, but I did have them just like that, but commented out. I tried all sorts of things, but it was blind experimentation. This info about how Dispose works in this case is super crucial, I'm so glad you posted this! Thanks a lot. I'm going to try this out too.

[Aaron McIver]

@SamusArin For clarification, setting to null if you call Dispose; otherwise calling Close should suffice without the need to set to null as you have stated.

[samus]

I'll give that a shot, I'm interested to see if will alleviate this situation (I got working now, but I still want to verify this). This is good info in general, thank you.

[samus]

Well, Bobson's answer above cleared up my confustion/uncertainty with the using statments behavior, and b/c every time these concerns come up people overwhelmingly suggest to use using statment, I'm going start as well. I really wanted to do clean up manually until I understood whats happening; I think I'm ready :)

[VladL]

@SamusArin I had the same problem like you. Closing, disposing and setting everything to null didn't help. Just manual GC call solved it. You are welcome :)

[ToolmakerSteve]

IMHO, this is a "sledgehammer" approach, that should be used sparingly. Good after a phase of a long running task, that you want to make sure is in a clean state before continuing. And is handy for debugging: if this helps, then somewhere there is a missing using or dispose. Try to get your code to work without this, then use it as an insurance policy, at a time where you don't mind an indeterminately long pause. If you find yourself doing this a lot, that suggests a deeper problem. Maybe even time to add a sub-process you can kill if needed (though that may complicate your code).

[ToolmakerSteve]

... On the other hand, I also ran into a case where this was the only way out: Had a lot of DirectX/XNA code mixed with .NET code, in a 32-bit process. Terrible fragmentation, until forcibly cleaned it up periodically with logic like this.

[ToolmakerSteve]

To expand on this answer: If Samus had added sr = null; after sr.Dispose();, then he could be sure that the sr from first iteration was never referred to again. Moving the declaration (with setting to null) inside of the loop is another way to be certain that each iteration is independent.

[ToolmakerSteve]

NOTE: in Samus case, this would not have helped. He did not do sr = null; after sr.Dispose();, so his code was still holding on to a reference to that disposed instance. Usually a variable goes out of scope before this is a problem, so the variable goes away on its own, but it is safest to always set to null after Dispose. (Presumably in Samus case, a WebException at the wrong moment kept sr from being set again in the second iteration, so the Disposed sr from first iteration was used, which is not allowed.)

[VladL]

@ToolmakerSteve it's rather a workaround, not a solution. I had to use it with external libraries a couple of times. In the own code it is not the way to fix exceptions. We used this code on the site with many visitors to keep it responsible. Otherwise if it runs using it's own scheduler, the site might be not responsible for a couple of seconds