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

You could clean your using statements up with the following : http://stackoverflow.com/questions/1329739/nested-using-statements-in-c-sharp — sbeskur, Dec 24 '12 at 17:05
Is there a specific reason you don't want to just go with a `using` statement? — Jeff Hubbard, Dec 24 '12 at 17:06
@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. — samus, Dec 24 '12 at 17:16
@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. — Bobson, Dec 24 '12 at 17:25
@Bobson Well ok then, I'll give the using statment a try, now that I know how it works. Sage tip, thank you. — samus, Dec 24 '12 at 19:20

score 2 · Answer 1 · edited May 23 '17 at 12:02

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?

edited May 23 '17 at 12:02

Community

1
1

answered Dec 24 '12 at 17:02

Tequilalime

611
9
29

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 Dec 24 '12 at 17:12
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. – Tequilalime Dec 24 '12 at 19:07
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 :) – samus Dec 26 '12 at 17:37

score 1 · Answer 2 · answered Dec 24 '12 at 17:27

1

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.

answered Dec 24 '12 at 17:27

Bobson

13,498
5
55
80

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. – samus Dec 24 '12 at 19:16
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 Jan 16 '17 at 18:03

score 0 · Answer 3 · answered Dec 25 '12 at 07:42

0

If your first run is ok and you get problems at some next runs, so it's most probably because the garbage collector didn't clean up closed objects. Do following in the end of finally block.

GC.Collect();
GC.WaitForPendingFinalizers();

answered Dec 25 '12 at 07:42

VladL

12,769
10
63
83

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 Dec 26 '12 at 17:33
@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 :) – VladL Dec 26 '12 at 17:42
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 Jan 16 '17 at 17:28
... 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 Jan 16 '17 at 17:33
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.) – ToolmakerSteve Jan 16 '17 at 18:06
@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 – VladL Jan 19 '17 at 10:57

Aaron McIver · Accepted Answer · 2014-11-11T04:14:16.710

-2

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

edited Nov 11 '14 at 04:14

answered Dec 24 '12 at 16:54

Aaron McIver

24,527
5
59
88

2

-1: setting to `null` has no effect, and sometimes even a negative effect. – John Saunders Dec 24 '12 at 17:00
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) ? – samus Dec 24 '12 at 17:04
1

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`. – Aaron McIver Dec 24 '12 at 17:05
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 Dec 24 '12 at 17:06
@Aaron McIver "Setting to null was in conjunction to calling Dispose"... Did you mean "to calling Close" ? – samus Dec 24 '12 at 17:25
@Aaron McIver No problem, thank you. Have a good holiday, too !) – samus Dec 24 '12 at 17:28
@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. – samus Dec 24 '12 at 17:36
1

@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. – Aaron McIver Dec 24 '12 at 23:14

Properly disposing a WebRequest and StreamReader

4 Answers4