What’s Wrong with this code – #4

Here’s another one for you. This segment of code gets the list of all responses to be sent and sends them.

public void TransmitResponse(ArrayList responses, 
                             StreamWriter streamWriter)
   foreach (DataResponse response in responses)
      NetworkResponse networkResponse = response.GetNetwork();
   GC.Collect();      // clean up temporary objects

Three questions:

1) What’s wrong with this code?
B) Why?
III) What should you do instead?

Comments (37)

  1. brandon says:

    1. Responses is not being closed. streamWriter is not being closed, they should both be in a using block, unless you do not wish to dispose your streamWriter.

    2. I prefer to declare variables that are involved in an iteration outside the looping block so you don’t have the overhead of creating the object each time.

    3. I see no reason to call GC.Collect() here.

    I would write this piece of code like this:

    NetworkResponse networkResponse;

    using(responses) {

    using(streamWriter) {

    foreach (DataResponse response in responses) {

    networkResponse = response.GetNetwork();





  2. 1.)

    The code has resource leaks. Particularly, it leaves network connections open after its done with them.


    It is trying to rely on an explicit garbage collection call to do its resource clean up. However, at the point GC.collect is called the NetWorkResponse instance used in the last loop iteration is not garbage: theres a reference to it left in the current stack frame. This resource will not be collected during garbage collection. Also, even if GC.Collect is run, all of the items that are garbage might not be collected. For example, during processing of the loop, garbage collection may occur and the current NetworkResponse may get moved up to generation 1. When the explicit collection occurs, it may only free items in generation 0, ignoring the item in the first generation.

    3) The code should explicitly call dispose on networkResponse. The easiest way to do this is to wrap the loop body in a using clause.

  3. Wedgebert says:

    I don’t see a reason to close responses or streamWriter here as they’re both parameters to the method, they could be used elsewhere.

    GC.Collect is probably bad for performance.

    There is no check to see if GetNetwork retured a value and it could be null.

    No check to see if streamWriter is null or is even open.

  4. Wedgebert says:

    You can’t know if you need to call Dispose on NetworkResponse? GetNetwork might return a reference to an already existing object (although it might should be a property if that is the case).

  5. Jeff Parker says:

    The number one thing wrong is calling GC.Collect()

    Let the garbage collector do it’s job.

    Monitor performance see if things are going into Gen 2, You may want to consider nulling the objects or you may want to consider reususing the objects. Unfortunately reusing in this case might be like shooting youself in the foot, because it is a stream you have no idea how big that stream is going to be each time. So reuse could be also bad.

  6. The code gives the impression that is not the case. I wouldn’t make that change without having access to source code or documentation.

    Its just my educated guess.

  7. JD says:

    1) Possible to throw InvalidCastException.

    2) Because an ArrayList is being passed in that can contain references to any type.

    3) Use typed list as argument.

  8. G. Man says:

    Some of you guys are calling Dispose on an ArrayList. As far as I know, ArrayList does not even implement IDisposable. You are even doing a "using" on an ArrayList? Bizarre.

    Furthermore, you should not close or dispose objects that are passed as parameters. In particular that stream is almost certainly used after your method returns.

    Looks like "whats wrong with this code" has generated even worse code than what you originally had.

  9. I’m going for most of what’s been said here and the fact that you failed to check whether the ArrayList responses is null before iterating over it with foreach.

  10. JD says:

    Your are right Omer. Maybe I should change:

    1) Capability to throw multiple exceptions

    2)responses could be null, streamWriter could be null, and responses could contain references to objects other than DataResponse.

    3) Use asserts.

  11. KC says:

    If the caller deals with any possible exceptions generated by this method, then the only glaring error is GC.Collect(). If that’s the case then only (1) below applies. If we are expected to catch other errors then (1) – (4) applies.

    1) No need for GC.Collect() here. Calling GC.Collect could force a collection of the temporary NetworkResponse objects in the body of the foreach loop. A lot depends on wether Send is a synchronous or asynchronous call. If it’s synchronous then there are no problems. If it is then a whole lot of errors including multiple threads trying to access the same StreamWriter could occur.

    2) The foreach loop could throw a ClassCastException. There is no guarantee that the ArrayList contains objects of type DataResponse.

    3.) networkResponse.Send(streamWriter) may generate an exception if networkResponse is null.

    4.) streamWriter may be null or closed.

    Fix could be:

    public void TransmitResponse(ArrayList responses, StreamWriter streamWriter)


    if (streamWriter != null){

    foreach (object obj in responses)


    DataResponse response =

    obj is DataResponse ?

    (DataResponse)obj : null;

    if (response != null){

    NetworkResponse networkResponse

    = response.GetNetWork();

    if (networkResponse != null){







    Assumption here is that networkResponse will deal with a closed streamWriter. If not then we probably want to propagate any generated exception to the caller.

  12. Sean Chase says:

    You should check the state of streamWriter because you are assuming it is open and not null. You should use a try/finally block and write "streamWriter.Close();" in the finally block. You should not call GC.Collect(). Also, you probably shouldn’t use ArrayList because ArrayList is inherently slow because the compiler can’t optimize all of the virtual methods.

  13. Optimzing away the virtual method calls to ArrayList and to its enumerator would probably not result in a noticable performance gain. The network operations it performs are orders of magnitude larger than a virtual dispatch call, and completly dominate the methods execution time.

  14. krishuggins says:

    I beleive that you cannot pass a streamwiter reference over the network, so the call to send should really pass a string, so the 2nd parameter should be converted to a string or bytes.

    Contrary to what others are saying, you should not dispose of the writer or the responses. The method that created these objects should dispose of them. The method name suggests transmiting responses, not closing responses.

    Also, you should not necessarily dispose of the NetworkResponse object because those belong to the responses in the ArrayList. The responses are likely burdenend with the lifecycle of these objects unless their documentation says otherwise.

  15. Fuman Chu says:

    public void TransmitResponse(ArrayList responses,

    StreamWriter streamWriter)


    foreach (DataResponse response in responses)


    using (NetworkResponse networkResponse = response.GetNetwork())






    I don’t know what the NetworkResponse object is – does it properly reset streams? In any case forced GC is seldom a good idea, such as this case.

  16. Eric Wilson says:

    GC.Collect is even less useful than many of hte posters noted. In this case, the only object in this method can be released is the enumerator silentlt created behind the scenes for the foreach statement (assuming this is not written with v2.0. In that case, I believe the enumerator is stack based and you gain even less). GC.Collect will NOT release any of the temp response objects because they are still referenced by the ArrayList.

  17. Thomas Eyde says:

    I think there isn’t more to say, except that GC.Collect does not belong here:)

    I don’t pay much attention to the untyped list, but I think I would use IList instead. If this is a public part of some library, then I would argue for a ResponseList or Collection. If not, then you should have the proper tests in place to secure the type of objects in the list. That goes for nulls in the list and the list itself, as well.

    Isn’t the rule that every resource you open, you should also close? I can’t see we are opening anything. It’s difficult to know if GetNetWork returns something we are expected to close or whether it’s IDisposable so we can using it.

    A question for the crowd: How should we write code to communicate that something is IDisposable and using is expected, or that a call to Close is expected?

    The StreamWriter is not used in this context, only passed on. The one who use it should check it for null, closed, etc.

    The method should be GetNetworkResponse, not GetNetWork.

    Finally: Why not just send a list of NetworkResponses?

  18. JD says:

    Letting it sink in further.

    1) GC.Collect does not belong in this method.

    2) Because the caller thinks that all they are doing is sending data (from the name), but actually they are sending data AND doing a GC collect (side effect).

    3) Remove the GC.Collect

  19. Gabe A says:

    I have a question: isn’t streamwriter being passed by reference? If so, then couldn’t the stream theoretically change during the foreach loop? Seems a little problematic to me.

    I agree with others that any I/O methods should be closed. Any casting done from an ArrayList should be checked for null and cast tested and I never use GC.Collect (never had to..).


  20. Sim says:

    1) C#

    2) It is the wrong language

    3) Use java 😉

  21. Matthew says:


    Thankyou for your valuable contribution.

  22. Eric Newton says:

    Biggest problem I see is the ArrayList passed in isnt guaranteed to be a DataRresponse

    Solution: change the type of responses from ArrayList to DataResponse[]

    2) I think maybe the networkResponse.GetNetwork() call should be wrapped in a using, without knowing what it does, so not sure if its instantiating or would need to dispose of that resource)

  23. Darren Oakey says:

    This is a hard one – at first glance it seems that the problem must be one to do with resources – and you’d think that the answer is to do a using (networkResponse), and you are tempted to shy away from GC.Collect

    HOWEVER… thinking a little more – the nomenclature denies that – if the line was networkResponse = response.CreateNetwork – it would be obvious, you should use using, but since it is GET network – it’s clear that you would be _completely wrong_ to call a dispose on the network response – because it might be returning a variable that is used again.. (For instance, imagine the following lines exist above us:

    TransmitResponse( responses, firstListener );

    TransmitResponse( responses, secondListener);

    It is completely NOT your option, without knowing a hell of a lot more, to call dispose! The get probabl returns an internal connection variable, which doesn’t ever get reallocated.

    Likewise with the streamwriter – you don’t know who will use the writer after you, so you cannot dispose it.

    So – what is wrong? Well – this routine could fail in a number of ways.

    * responses could be null

    * elements of responses could be null

    * elements of responses could be not DataResponses

    * stream writer could be null

    * stream writer could be in some non-readable state

    * response.GetNetwork could return null

    So, you need to address all of these. I’d suggest:

    public void TransmitResponse( nonnull<List<nonnull<DataResponse>>> responses, nonnull<StreamWriter> streamWriter )


    Ensure.IsOpenStream( streamWriter );

    foreach (nonnull<DataResponse> response in responses)


    nonnull<NetworkRespnose> networkResponse = response.Call.GetNetwork();

    networkResponse.Call.Send( streamWriter );


    [More here later]


    So – the final thing – what about GC.Collect? That’s obviously wrong, isn’t it?

    Well.. maybe. But.

    We don’t know what’s going on inside any of this stuff. We absolutely aren’t allowed to dispose things, because we are calling a function called "Get" – which doesn’t hint at creation, so we have no information that we "own" the result – in fact quite the opposite – the word Get implies that we DO NOT own the result. So we can’t dispose it.

    However – each of these network things MAY create connections, and hold limited resources open. We discover this tragically, when the production systems fall over. This is a BAD THING. Now – we COULD check and find that the network stuff does to a creation etc, and find that we can dispose it. However, we may not have the source, and we definitely don’t know who’s calling our function, and we definitely don’t know what changes some idiot is going to do to it in the future so we have no idea what the side-effects of doing this will be…

    so, we have exactly one option – put in a GC.Collect(). Of course, that’s fairly pointless – because it just tells the thread to go collect everything. so, (even scarier to those "yes the garbage collection works" pundits) we need to follow it with GC.WaitForPendingFinalizers.

    Now, all of you (us) know that the dispose option is the much better alternative – but in a lot of cases, it cannot possibly be safe to do so – you just don’t have that information. The fact of life is – if you want to stop the server falling over, and you don’t want to potentially introduce far worse bugs, and you don’t have a lot of time to investigate – the GC.Collect + WaitForFinalizers is your only sensible option – it changes an unpredictable state to a predictable one, won’t cause any further problems – and if it works in your tests, will continue to work in production.

  24. Maksym says:

    May be the problem is if we have an exception in response.GetNetwork() or in networkResponse.Send(streamWriter) then in the catch block we will not be able to answer how much of DataResponses had been processed. We need to mark or remove from list successfully processed DataResponses.

  25. Sachin says:

    If we need to use GC.Collect it should be in the finally block that way the GC.Collect() would be called irrespective of the fact whether the above code throws an exception or not & perhaps GC.WaitForPendingFinalizers() should be added as well.

  26. Chris R. Timmons says:

    public void TransmitResponse(ArrayList responses,

    StreamWriter streamWriter)


    // Assume a multithreaded execution environment.

    lock (this)


    // Check the inputs for basic validity.

    Debug.Assert(responses != null, "Method TransmitResponse: ArrayList ‘responses’ parameter cannot be null.");

    Debug.Assert(streamWriter != null, "Method TransmitResponse: StreamWriter ‘streamWriter’ parameter cannot be null.");

    Debug.Assert(streamWriter.BaseStream.CanWrite, "Method TransmitResponse: StreamWriter ‘streamWriter’ parameter must be open and writeable.");

    foreach (object obj in responses)


    // Don’t assume the responses ArrayList contains

    // only DataRespone instances. Skip those that

    // aren’t.

    DataResponse response = obj as DataResponse;

    if (response == null)


    // Nothing is given about GetNetwork()’s behavior.

    // For simplicity’s sake, just check for a null return

    // value. Could also be wrapped in a "using" statement

    // for resource protection, and a "try/catch" block to

    // handle any necessary exceptions.

    NetworkResponse networkResponse = response.GetNetwork();

    if (networkResponse == null)







  27. murphee says:

    >2. I prefer to declare variables that are

    >involved in an iteration outside the looping

    >block so you don’t have the overhead of creating

    >the object each time.

    Sigh… why doesn’t this bit of optimization folkore vanish from the earth…

    OK: doing a

    Network xyz;

    does not translate to an executable statement;

    it tells the compiler/VM to make the Stackframe big enough to hold a reference… period. Nothing else. The fact that you can define your variables anywhere in the code is just a convenience introduced in C++,… there is no overhead.

  28. Anon says:

    Chris, does networkResponse.Send() throw an exception?

  29. Nick says:

    Obviously the issue is with the GC.Collect. You ought to be using IDisposable and the using directive… or a Try/Finally block.

    The funny thing is that I actually did a code review at my last client site where someone did this! He threw GC.Collect everywhere. He was an ex hard core C++ programmer who absolutely hated garbage collection and wanted deterministic finalization, and thought this was the route to get it.

    Instead of taking the route that I did which was to bitch about the lack of determinstic finalization, but live with garbage collection and learn how to make proper use of the functionality… he decided to fight it. Of course GC.Collect is not synchronous (unless you call WaitForPendingFinalizers, so he wasn’t getting what he wanted anyway.

    I ended up spending about half a day showing him best practices.

  30. Josh Charles says:

    Both attributes are being passed in by reference, and the Arraylist can contain references to any number of other objects. If you wanted to dispose of each "response" as you used them there is a much better method.

  31. Uwe Keim says:

    What about this answer: NOTHING :-)

  32. MBA says:

    Helpful For MBA Fans.