Closing a thread in a function is a bad idea?

Question

I wrote the code today and it was changed by another developer who said that it is more secure. I am not sure if this is correct, since I don’t see the benefits of what was done here, these are code examples

public byte[] ReadFile(Stream stream) { byte[] result = null; try { // do something with stream result = <result of operation> } finally { stream.Close(); } return result; } 

it has been changed to

 public byte[] ReadFile(Stream stream) { byte[] result = null; // do something with stream result = <result of operation> return result; } 

Am I new to C # if the thread is not closed when you finish with it?

Answer 1

In general, the first version will be bad.

Yes, the thread should be closed, but preferably with the same code (the same method) that opened it. This is called separation of concerns, and it makes mistakes and confusion much less likely.

So either

• your ReadFile() takes, for example, a string fileName and opens and closes the stream, or
• you leave it to the caller.

Your method (second version) should be used as follows:

 using (var reader = new FileReader(...)) { // maybe some pre-reading var r = ReadFile(reader); // maybe some post-reading } 

Note that the second approach also makes the method more reusable.

Answer 2

There is no right answer in this question, because it depends on the architecture of your application.

I would say yes, because if a stream not created in this function, but only used, therefore closing it allows the caller to be prevented.

But I repeat, it depends on your application architecture.

Answer 3

Who will open the door, do not forget to close it.

Therefore, it is better to close the thread in the method that opened it.

Answer 4

Typically, the creator of the Stream should be closed, ideally, using it with a block:

 using (var myStream = getMeAStream()) { ReadFile(myStream); // If you want to be really sure it is closed: myStream.Close(); // Probably not neccessary though, since all // implementations of Stream should Close upon Disposal } 

Answer 5

The thread was transferred to this function by something else, you are not responsible for the function to close it, it depends on the calling code to solve this problem.

The reason is that it may take another operation (for example, reset or another reading) to execute this function, and if you close it, you will throw an exception.

I used to reuse the code, as it was several times a day, until finally someone listened to me.

I have seen at least one serious error caused by such code, so its a bad idea in general

Answer 6

The code check was correct - never do such a thing (or if something like this is required by external code, then this code is probably not designed properly - there are exceptions, but for things like threads that are practically absent there , and some default patterns should always be followed).
in most cases they use such streams (from "outside") ...

 using(Stream stream = new ...) { ...call your method } 

(or, for example, the reader gets rid of it, but he recommended that you do something similar anyway - the equivalent uses the finally block, but it boils down to that)

... basically the calling function will never know if you deleted it inside or not until it crashes. If the “both sides” agree to something similar, then this is still unacceptable, but may go unpunished.

Answer 7

This is not just a problem with C #. After calling the ReadFile function, do you need to use the stream for other operations? If you could not close the stream after reading the file. It’s better to close the threads when you finish using them, but usually I prefer to close them in the same context as opening them, because there is a place where you know if you need a thread for another operation:

 Stream s = open_the_stream() try { ReadFile(s)... } finally { s.Close(); } 

In any case, close the stream when you finish using it.