Question on Socket disconnect handling
Bruce
I have a question on what is the cleanest way to handle disconnects in
my socket handling code.
I have an application in which there is a class that once initiated
calls socket.BeginReceive() in whose callback I do some processing and
call BeginReceive again (so I read in a loop.)
This works great until I decide to disconnect. In the disconnect code,
I close the socket and shut it down. But the BeginReceive thread may
still be going on and I get a Socketexception or a
ObjectDisposedException in BeginReceive or EndReceive based on the
timing. I don't care about this error at this stage since I have
closed the socket.
But while running my app under load, I see a large number of CLR
exceptions/sec (perf counter) because of the constant disconnects I
do. (intentional)
My question is, since exceptions are expensive in C#, I should keep
them to 0 most of the time right? But in the above case, I am getting
legitimate exceptions that I handle.
1. Should I not worry about the exception rate?
2. Or, should I place some flag all over the code so that I check the
flag before doing EndReceive or BeginReceive? And I set the flag in
the Disconnect code. (race conditions can still exist)
I am trying to make my app scale as much as I can.
Thanks
Bruce
Peter Duniho
wrote:
> Hi
> I have a question on what is the cleanest way to handle disconnects in
> my socket handling code.
> I have an application in which there is a class that once initiated
> calls socket.BeginReceive() in whose callback I do some processing and
> call BeginReceive again (so I read in a loop.)
> This works great until I decide to disconnect. In the disconnect code,
> I close the socket and shut it down. But the BeginReceive thread may
> still be going on and I get a Socketexception or a
> ObjectDisposedException in BeginReceive or EndReceive based on the
> timing. [...]
It sounds as though you are doing the disconnect incorrectly. One
endpoint or the other will initiate the disconnect by doing a graceful
closure. In .NET, this means (for the Socket class) calling the
Shutdown() method. Once you've called that, you must continue to receive
data until you get a 0-byte receive, indicating the end of the stream.
Only at that point should you be calling Close() on the Socket instance.
And of course at that point, you would not try to receive any more data.
If you do it correctly, you should get no exceptions thrown.
Pete
Ken Foskey
The solution that I took to this was to simply use a thread with receive
(not async) and timeout. So every 5 seconds it loops around and checks
a boolean variable and exits the loop cleaning up.
Bruce
wrote:
> On Sat, 07 Feb 2009 23:09:26 -0800, Bruce <bruce.james....@gmail.com>
Thanks for the reply Pete. Yes, I was doing the incorrect thing - now,
I have modified my code to first call Shutdown and wait for zero bytes
to be got before calling socket.close.
I have a related question - say sending a certain message makes the
server disconnect my client
I call
socket.BeginSend("badMessage",.....,callback..)
In the callback method, I call socket.EndSend(ar). Now, I observe that
sometimes I get an ObjectDisposedException here. I assume there could
be a legitimate race condition between the callback getting called/
EndSend called and the socket being disconnected by the server?
And I have to absorb the exception or choose not to call EndSend?
Thanks
Bruce
Peter Duniho
wrote:
> [...]
> In the callback method, I call socket.EndSend(ar). Now, I observe that
> sometimes I get an ObjectDisposedException here. I assume there could
> be a legitimate race condition between the callback getting called/
> EndSend called and the socket being disconnected by the server?
> And I have to absorb the exception or choose not to call EndSend?
Yes. Well, actually...there's at least one other option: handle all
outstanding EndSend() calls when you get the 0-byte receive, before you
call Socket.Close(), and then ignore them in their own callbacks.
To me, that's the "cleanest" way to deal with the issue, since it avoids
any exceptions. But it's a bit of extra work for no real apparent gain as
far as I can see.
You should definitely still call EndSend() though. So just ignore the
exception that occurs there if you know the socket should have already
been closed.
Pete
Bruce
wrote:
> On Sun, 08 Feb 2009 02:24:44 -0800, Bruce <bruce.james....@gmail.com>
Thanks Pete.
> But it's a bit of extra work for no real apparent gain as far as I can see.
Under high load, I do not want to see a large number of CLR exceptions
and that is why I am trying to resolve this.
Since I am anyway throwing away the connection, what is actually wrong
with not calling EndSend at all?
Peter Duniho
wrote:
> Under high load, I do not want to see a large number of CLR exceptions
> and that is why I am trying to resolve this.
Unless you have measured a real performance issue due to the exceptions, I
think you should not worry about it. Exceptions can be extremely
expensive when the debugger is running. But without the debugger
attached, they are only "sort of expensive". As long as you're not seeing
a huge number of them over and over, I doubt it will affect your
throughput at all.
> Since I am anyway throwing away the connection, what is actually wrong
> with not calling EndSend at all?
The short answer is: because there's nothing in the documentation that
says you are allowed to not call EndSend(). The general async pattern in
.NET requires balanced Begin...() and End...() method calls.
As far as more specific reasons go, .NET is required to keep the data
structures around that allow you to successfully call EndSend() until you
do. If you don't call EndSend(), that can lead to resource allocation
issues. In some sense, EndSend() is an alternate pattern to IDisposable
for operations. If you haven't called EndSend(), you haven't properly
disposed of the operation that you started.
If you just lose track of the IAsyncResult that was returned by
Begin...(), then it's possible a finalizer on the operation will
eventually run and clean things up for you. But that's not something you
should rely on. Code that depends on the finalizer is code that's broken.
Pete
Bruce
wrote:
> On Sun, 08 Feb 2009 09:39:48 -0800,Bruce<bruce.james....@gmail.com>
Thanks Pete. I think I will just take the hit of the exceptions in the
send case. I should not be relying on the exceptions counter to
determine whether something "bad" happened on my client.
To clarify the earlier point, a disconnect is only truly complete when
EndReceive returns 0 bytes upon which I call socket.Close().
So, a client disconnect is an asynchronous operation and I should wait
for it to 'complete' (by say, having an event after socket.close())
and only then I should reuse my socket read and write buffers.
Is that right?
Bruce
Peter Duniho
wrote:
> Thanks Pete. I think I will just take the hit of the exceptions in the
> send case. I should not be relying on the exceptions counter to
> determine whether something "bad" happened on my client.
> To clarify the earlier point, a disconnect is only truly complete when
> EndReceive returns 0 bytes upon which I call socket.Close().
> So, a client disconnect is an asynchronous operation and I should wait
> for it to 'complete' (by say, having an event after socket.close())
> and only then I should reuse my socket read and write buffers.
>
> Is that right?
I'm not sure what you mean by "disconnect is an asynchronous operation".
Network i/o isn't inherently synchronous or asynchronous; it's just i/o.
You can process it either way.
If you are processing network i/o with asynchronous techniques, then the
disconnect is asynchronous as well. But, you could easily make it
synchronous simply by using blocking methods to do the disconnect (i.e.
call Shutdown(), then Receive() until 0 bytes, then Close(), in sequence
all in the same thread).
But, other than that, yes...it sounds like you've got the idea.
Pete
Bruce
wrote:
> On Mon, 09 Feb 2009 17:22:20 -0800,Bruce<bruce.james....@gmail.com>
> wrote:
>
> > Thanks Pete. I think I will just take the hit of the exceptions in the
> > send case. I should not be relying on the exceptions counter to
> > determine whether something "bad" happened on my client.
> > EndReceive returns 0 bytes upon which I callsocket.Close().
> > for it to 'complete' (by say, having an event aftersocket.close())
>
> > Is that right?
>
> I'm not sure what you mean by "disconnectis an asynchronous operation".
> Network i/o isn't inherently synchronous or asynchronous; it's just i/o.
> You can process it either way.
>
> synchronous simply by using blocking methods to do thedisconnect(i.e.
> call Shutdown(), then Receive() until 0 bytes, then Close(), in sequence
> all in the same thread).
>
> But, other than that, yes...it sounds like you've got the idea.
>
> Pete
Hi Pete
Thanks for patiently answering all my questions. I have one final
question on this topic. (Sorry, the MSDN documentation is just not
enough to know the exact cleanup pattern)
1. Should I socket.Shutdown with Send alone since I am trying to
receive 0 bytes? Or should I socket.Shutdown(Both)?
2. Can I reuse my read/write buffer for another socket after Shutdown
instead of waiting for getting for 0 bytes and doing a socket.Close()?
(since though buffer can be accessed concurrently, since I have
disabled send,receive, nothing should be written to the buffer by the
shutdown socket)
Thanks
Bruce
Peter Duniho
wrote:
> Thanks for patiently answering all my questions. I have one final
> question on this topic. (Sorry, the MSDN documentation is just not
> enough to know the exact cleanup pattern)
The .NET docs are sparse, to be sure. But, a lot of the .NET model
follows the standard Winsock model, which is much better documented. So,
reading through the Winsock docs on MSDN would probably be helpful to
you. As would reading through the Winsock FAQ
(http://tangentsoft.net/wskfaq/).
> 1. Should I socket.Shutdown with Send alone since I am trying to
> receive 0 bytes? Or should I socket.Shutdown(Both)?
The initiator of the disconnect should use Send. The other endpoint will
then get a 0 byte receive once all the data the initiator had sent has
arrived at the remote endpoint; once the remote endpoint is done sending
all the data it needs to send, it can call Shutdown() with Both. After
calling Shutdown() with Send, the initiator will continue to receive data
until _it_ gets a 0 byte receive.
In this way, both endpoints can reliably read from the socket until the
end-of-stream indication (0 byte receive), with confidence that by the
time they close the socket, both ends really are done with the connection.
> 2. Can I reuse my read/write buffer for another socket after Shutdown
> instead of waiting for getting for 0 bytes and doing a socket.Close()?
> (since though buffer can be accessed concurrently, since I have
> disabled send,receive, nothing should be written to the buffer by the
> shutdown socket)
You should not shutdown receives on a socket until you've actually reached
the end-of-stream (i.e. had a receive operation complete with 0 bytes).
Otherwise, you may (and probably will) lose data.
That said, if you don't care about losing data, then sure...call
Shutdown() with Both instead of Send. This _should_ (I haven't tested it
myself) cause any outstanding read operation on the socket to complete
(probably with an exception). Once you've completed those read operations
(e.g. by calling Socket.EndReceive()), you can reuse the buffers that were
used for those operations.
Don't reuse a buffer until the operation that was using it has completed
somehow. Otherwise you don't have any guarantee that it won't be written
to by the Socket you originally used it with.
Pete
Bruce
wrote:
> On Mon, 09 Feb 2009 18:30:12 -0800,Bruce<bruce.james....@gmail.com>
> wrote:
>
> > Thanks for patiently answering all my questions. I have one final
> > question on this topic. (Sorry, the MSDN documentation is just not
> > enough to know the exact cleanup pattern)
>
> The .NET docs are sparse, to be sure. But, a lot of the .NET model
> follows the standard Winsock model, which is much better documented. So,
> reading through the Winsock docs on MSDN would probably be helpful to
> you. As would reading through the Winsock FAQ
> (http://tangentsoft.net/wskfaq/).
>
> > receive 0 bytes? Or should Isocket.Shutdown(Both)?
>
> then get a 0 byte receive once all the data the initiator had sent has
> arrived at the remote endpoint; once the remote endpoint is done sending
> all the data it needs to send, it can call Shutdown() with Both. After
> calling Shutdown() with Send, the initiator will continue to receive data
> until _it_ gets a 0 byte receive.
>
> end-of-stream indication (0 byte receive), with confidence that by the
>
> > 2. Can I reuse my read/write buffer for anothersocketafter Shutdown
> > instead of waiting for getting for 0 bytes and doing asocket.Close()?
> > (since though buffer can be accessed concurrently, since I have
> > disabled send,receive, nothing should be written to the buffer by the
> > shutdownsocket)
>
> the end-of-stream (i.e. had a receive operation complete with 0 bytes).
> Otherwise, you may (and probably will) lose data.
>
> That said, if you don't care about losing data, then sure...call
> Shutdown() with Both instead of Send. This _should_ (I haven't tested it
> (probably with an exception). Once you've completed those read operations
> used for those operations.
>
> Don't reuse a buffer until the operation that was using it has completed
> somehow. Otherwise you don't have any guarantee that it won't be written
>
> Pete
Great! Thanks for all the help. I will definitely readup the Winsock
documentation for gaining a better understanding.
Bruce
Matthew
is completely wrong. It's not a matter of performance. It's a matter
of correctness. You should never call a method on a resource that has
already been closed. Blindly calling EndSend() because one part of some
documentation says so is not the right thing to do when a fundamental
rule of programming contradicts it. If you can't guarantee that a
resource is still available when you call a method on it, then you
shouldn't call the method. In any case, you should be waiting for all
outstanding asynchronous callbacks to finish, in which case that won't
happen.
Catching and swallowing the disposed exception on a different thread
(since asynchronous callbacks run on a worker thread) from the thread
that calls Dispose is not safe because the popular form of the
IDisposable pattern is not thread safe. That means that even if you
don't get an exception, you may have still had some unpredictable effect
on some resource that has already been closed if the Dispose method is
running at the same time as an asynchronous callback. Take this
example:
1. Socket.Dispose is called (on main thread)
2. asynchronous callback runs (on worker thread)
3. asynchronous callback calls Socket.EndSend and no exception occurs
because the socket's `disposed' flag has not been set yet
4. Socket.Dispose sets disposed = true
5. Socket.Dispose returns (on main thread)
6. main thread proceeds to close resources, some of which are accessed
by the asynchronous callback
7. asynchronous callback (on worker thread) accesses resources that have
already been closed
The workaround for this is to:
- count the number of outstanding asynchronous callbacks in a
thread safe way
- block any callbacks from proceeding then wait for all outstanding
callbacks to finish _before_ you call Dispose/Close
This is a general issue with all asynchronous callbacks in the .NET
framework.
*** Sent via Developersdex http://www.developersdex.com ***
Peter Duniho
> No, he should not simply call EndSend() and ignore the exception. That
> is completely wrong. It's not a matter of performance. It's a matter
> of correctness. You should never call a method on a resource that has
> already been closed.
Says who?
> Blindly calling EndSend() because one part of some
> documentation says so is not the right thing to do when a fundamental
> rule of programming contradicts it.
There is no such "fundamental rule of programming".
> If you can't guarantee that a
> resource is still available when you call a method on it, then you
> shouldn't call the method.
And for blocking calls that will throw the exact same exception when the
resource is closed before the blocking call returns? How are you supposed
to "guarantee" that the resource is still available before you call it,
given that the close happens after?
> In any case, you should be waiting for all
> outstanding asynchronous callbacks to finish, in which case that won't
> happen.
Huh? What are you talking about? The whole point is to call the End...()
method in the callback. By definition, if you're calling the End...()
method, the "outstanding asynchronous callback" HAS finished. There's no
"waiting" involved.
> Catching and swallowing the disposed exception on a different thread
> (since asynchronous callbacks run on a worker thread) from the thread
> that calls Dispose is not safe because the popular form of the
> IDisposable pattern is not thread safe.
What "popular form"? Assuming there really is one that precludes
thread-safety, why is that at all relevent for a _specific_ type in .NET?
Socket is documented as being thread-safe. That had better include its
Dispose() method, otherwise it has a bug.
> That means that even if you don't get an exception,
What do you mean "even if you don't get an exception"? If the object is
disposed, you _will_ get an exception.
> you may have still had some unpredictable effect
> on some resource that has already been closed if the Dispose method is
> running at the same time as an asynchronous callback. Take this
> example:
>
> 1. Socket.Dispose is called (on main thread)
Socket is thread-safe.
> 2. asynchronous callback runs (on worker thread)
Socket is thread-safe.
> 3. asynchronous callback calls Socket.EndSend and no exception occurs
> because the socket's `disposed' flag has not been set yet
Socket is thread-safe.
> 4. Socket.Dispose sets disposed = true
Socket is thread-safe.
> 5. Socket.Dispose returns (on main thread)
Socket is thread-safe.
> 6. main thread proceeds to close resources, some of which are accessed
> by the asynchronous callback
Socket is thread-safe.
> 7. asynchronous callback (on worker thread) accesses resources that have
> already been closed
Socket is thread-safe. Your above scenario isn't possible, because the
class is thread-safe, promising that access from multiple threads won't
cause a problem. If you have some reason to believe this is not true, you
need to file a bug report, rather than making erroneous statements about
how one should use the Socket class.
> The workaround for this is to:
> - count the number of outstanding asynchronous callbacks in a
> thread safe way
> - block any callbacks from proceeding then wait for all outstanding
> callbacks to finish _before_ you call Dispose/Close
>
> This is a general issue with all asynchronous callbacks in the .NET
> framework.
If that's true, where is your bug report, confirmed by Microsoft?
What _is_ a "general issue" is that you are _required_ to call the
End...() method once you call the Begin...() method. If you don't, you
will leak resources. And yes, it's entirely possible and expected for the
call to the End...() method to throw an exception. And no, there should
be no risk of a synchronization problem between the call to End...() and
Dispose(); by definition, classes that implement the asynchronous
operation model must be thread-safe, and thread-safe means that the object
can be safely used from multiple threads.
Note: being thread-safe does not necessarily mean sections of code are
synchronized; it simply means that the class guarantees that simultaneous
operations from multiple threads are safe and won't corrupt the data
structure.
If you have a code example that demonstrates a thread safety problem when
using the Begin.../End... async pattern, you should file a bug report.
Pete
Matthew
>> method on a resource that has already been closed.
>> documentation says so is not the right thing to do when
>> a fundamental rule of programming contradicts it
> There is no such "fundamental rule of programming".
I'm actually surprised you would say that considering you are concerned
with leaking memory. These two fundamental rules of resource
acquisition go hand in hand: close any resource you open (yours but I
agree) and don't access a resource after its been closed (mine). I
don't know how you could possibly appreciate one but not the other.
The contradiction is that you are saying to break the second rule in
order to satisfy the first. You are saying to potentially use a closed
socket object in order to close the socket object's own asynchronous
callback structures. That's a bit of a paradox.
I agree you must close both, and the Socket Dispose method should do
this. If the Socket class is implemented correctly, then its Dispose
method should free all associated resources, including those used by
EndSend. Think about it. If you get an exception when you call EndSend
then your EndSend never really ran successfully anyway, did it? That
means in the worst case you really are relying on Dispose to clean up
any remaining resources associated with the socket.
> And for blocking calls that will throw the exact same
> exception when the resource is closed before the blocking
> call returns? How are you supposed to "guarantee" that the
> resource is still available before you call it,
> given that the close happens after?
That would be another bug. If you open a resource, you own it, and if
you should always later be able to close it without error.
> Huh? What are you talking about? The whole point is to
> call the End...() method in the callback. By definition,
> if you're calling the End...() method, the
> "outstanding asynchronous callback" HAS finished.
> There's no "waiting" involved.
The callback may do other things after it calls the End...() method. It
may set an event, for instance.
> Socket is documented as being thread-safe. That had
> better include its Dispose() method, otherwise it has a
> bug.
Microsoft doesn't reveal their source code, but it probably has a bug.
The world is not perfect. These things slip by.
> What do you mean "even if you don't get an exception"? If
> the object is disposed, you _will_ get an exception.
You hope you will, but like I said, the world is not perfect. Look at
the recommended IDisposable pattern and you will see that it is not
thread safe. It's likely Microsoft used the pattern in Socket and many
other classes without thinking, and they don't fix it because it doesn't
often cause problems. These thing slip by. That doesn't make them
right.
> Socket is thread-safe.
> Socket is thread-safe.
> Socket is thread-safe.
> ...
Just because the documentation says something, doesn't mean it's true.
You may wish it were true. I may wish it were true, but if the people
who developed the Socket class were really doing the job they should
have done, then they would have provided a way to cancel asynchronous
callbacks and wait for them to finish. It's normal and safe to provide
a means of canceling outstanding asynchronous operations and ensuring
that they complete before shutting down some subsystem.
> If you have some reason to believe this is not true, you
> need to file a bug report, rather than making erroneous
> statements about how one should use the Socket class.
I filed a report. They ignored it. The world is not perfect. It's
powered by money, greed, and impatience.
My statements are not erroneous. Microsoft is not as great as you wish
they were. No one is. If you want your system to work, sometimes you
have to take care in areas where the supposed gods have not.
Peter Duniho
>>> It's a matter of correctness. You should never call a
>>> method on a resource that has already been closed.
>>> Blindly calling EndSend() because one part of some
>>> documentation says so is not the right thing to do when
>>> a fundamental rule of programming contradicts it
>
>> There is no such "fundamental rule of programming".
>
> I'm actually surprised you would say that considering you are concerned
> with leaking memory. These two fundamental rules of resource
> acquisition go hand in hand: close any resource you open (yours but I
> agree) and don't access a resource after its been closed (mine). I
> don't know how you could possibly appreciate one but not the other.
Because the two are not related to each other. Leaking resources _is_ a
well-known, documented rule of programming. It is easy to demonstrate
program failures due to not following the rule. Your proposed rule isn't
anything like that.
> The contradiction is that you are saying to break the second rule in
> order to satisfy the first. You are saying to potentially use a closed
> socket object in order to close the socket object's own asynchronous
> callback structures. That's a bit of a paradox.
No, it's not. The resources associated with the asynchronous object
aren't owned by the object itself. They are owned by the IAsyncResult
implementation returned by the Begin...() method, and they are only
released by a call to the End...() method.
> [...]
>> Socket is documented as being thread-safe. That had
>> better include its Dispose() method, otherwise it has a
>> bug.
>
> Microsoft doesn't reveal their source code,
Completely untrue. Even before Microsoft released the .NET source, you
could inspect the implementation details with the Reflector tool. And of
course, Microsoft _has_ released the .NET source code. You are free to
look at it if you like.
> but it probably has a bug.
"Probably" doesn't cut it. Either you know there's a bug or you don't.
Now, as it happens, since I am in fact able to look at the implementation,
I can in fact see that there _may_ in fact be a bug. I haven't spent
enough time with the code to be sure -- it's fairly convoluted, at least
looking at it in Reflector, and it's possible the cleanup is handled
elsewhere in the dispose case -- but it does appear that EndSend() might
fail to cleanup the IAsyncResult implementation if the Socket has already
been disposed.
But even assuming the bug exists, that doesn't mean you should just write
code that assumes the bug is always there, and thus you don't need to
finish your asynchronous operation correctly. It means Microsoft has a
bug to fix, and when they do code that always calls EndSend() will start
working properly again.
> [...]
>> If you have some reason to believe this is not true, you
>> need to file a bug report, rather than making erroneous
>> statements about how one should use the Socket class.
>
> I filed a report. They ignored it.
Where's your bug report? Please provide a link to the Connect submission
you created. Assuming it was a proper bug report, with an appropriate
reproducible description (including a concise-but-complete code example),
we should be validating the bug, rather than wasting time on useless
arguments about whether one should follow the documentation or one's own
bliss.
> The world is not perfect. It's
> powered by money, greed, and impatience.
What's that got to do with the Socket class?
Pete
Matthew
>> are concerned with leaking memory. These two fundamental
>> rules of resource acquisition go hand in hand: close any
>> resource you open (yours but I agree) and don't access a
>> resource after its been closed (mine). I don't know how
>> you could possibly appreciate one but not the other.
> Because the two are not related to each other. Leaking
> resources _is_ a well-known, documented rule of
> programming. It is easy to demonstrate program failures>
> due to not following the rule. Your proposed rule isn't
> anything like that.
That is some combination of ignorant and insane. I hope others don't
follow this bad example. The two rules are _completely_ related. Let
me generalize the concepts of "opening" and "closing" to what they
really are, acquiring and releasing. When you acquire a resource, you
own it and no one else is allowed to access it unless you give them
permission. When you release that resource, you no longer own it, and
you lose the right to access it.
The rule of not touching a resource after you've released it is the
_reason_ ObjectDisposedException even exists! ObjectDisposedException
is not a boolean flag. It is an error letting you know that you did
something seriously wrong. The fact that you've gotten used to using it
with the Socket class is only evidence that the class was poorly
designed and you followed right along without question.
I know they didn't teach resource acquisition/release in my school, but
I learned it through experience. Perhaps it's getting low-level
background in C and C++ that really makes someone appreciate that these
same concepts still apply to sockets, files, and other unmanaged
resources in high-level languages.
Garbage collection and exception handling does not magically solve every
problem. It can hide many problems, but it doesn't fix them all. It
doesn't make the code correct. There's a difference between doing
something right with good principles that work well in many situations
and throwing something sloppy together that sort of gets the job done
but has to be massaged in awkard ways to get it to make sense.
> No, it's not. The resources associated with the
> asynchronous object aren't owned by the object itself.
> They are owned by the IAsyncResult implementation
> returned by the Begin...() method, and they are only
> released by a call to the End...() method.
And who do you think should own the IAsyncResult? The socket object
does (or should).
> than wasting time on useless arguments about whether one
> should follow the documentation or one's own
> bliss.
It's been worthwhile if it stops just one other person from following
your bad advice, if it gets that one other person to properly understand
the concept of resource acquisition and release.
I can see that there's no reasoning with you, but thanks for letting me
know the source code is out there. It may be interesting to check out,
but the rule that you should not access a resource that you have already
released still applies.
Peter Duniho
> [...]
> I know they didn't teach resource acquisition/release in my school, but
> I learned it through experience. Perhaps it's getting low-level
> background in C and C++ that really makes someone appreciate that these
> same concepts still apply to sockets, files, and other unmanaged
> resources in high-level languages.
lol...I've been programming three decades and have plenty of "low-level
background in C and C++". I understand these concepts just fine, thank
you. Something you've apparently failed to "learn through experience" is
that when you're told to match up calls to paired methods, you should
follow the docs.
So far, you've completely failed to support your claim of "fundamental
rule", either by documentation or example, and instead chosen to continue
with the personal insults. In my experience, that's the surest sign of
someone speaking from dogma rather than knowledge. The fact that you
can't actually provide the link to the bug report you claim to have made
further demonstrates your inability to produce actual information on the
topic.
> [...]
> I can see that there's no reasoning with you, but thanks for letting me
> know the source code is out there. It may be interesting to check out,
> but the rule that you should not access a resource that you have already
> released still applies.
That's just not a rule. It's not even practical to accomplish it for
asynchronous, thread-safe objects. The only way to do it would be to wrap
all your access on those objects with more synchronization, making it
pointless for the objects themselves to be thread-safe in the first place.
Pete
Matthew
the common sense of not accessing resources after they've been released,
but for anyone reasonable who's still interested, here is an example
that shows that catching ObjectDisposedException in an asynchronous
callback is not a thread-safe way to determine whether the socket has
already had Close/Dispose called on it. Besides the fact that such a
technique is sloppy, in order for it to even be safe, Close/Dispose
should not be able to return while there is an asynchronous callback in
progress (but this is not true). If there is an asynchronous callback
in progress when Close/Dispose runs, the call to EndXxx may have already
run (even though the callback has not finished everything it was doing),
in which case the callback will not catch an exception and will then
proceed to access other resources in a race with the cleanup code for
those resources in the main thread.
Take the example below where an event has Close/Dispose called on it,
which later causes the event's Set method to throw an
ObjectDisposedException (not the one expected from the socket).
BeginSend is used for the convenience of getting the callback to run
soon, but this could be any asynchronous operation. More realistically
it could be a BeginReceive that was still waiting for data to arrive
when it was time to shut down a subsystem.
using System;
using System.Collections.Generic;
using System.Threading;
using System.Net.Sockets;
namespace Problem
{
class Program
{
static void SendCallback(IAsyncResult ar)
{
Console.WriteLine("SendCallback entered");
SendCallBackArgs args =
(SendCallBackArgs)ar.AsyncState;
try
{
// it doesn't matter if other resources
// are accessed before or after EndSend()
// is called; catching
// ObjectDisposedException is not a
// reliable way to prevent accessing them
// after they have been released
//
// enable either case, and the result is
// the same
#if true
args.Socket.EndSend(ar);
// simulate timing that can cause callback
// and Close to race
Thread.Sleep(1000);
args.Event.Set();
#else
// simulate timing that can cause callback
// and Close to race
Thread.Sleep(1000);
args.Event.Set();
args.Socket.EndSend(ar);
#endif
}
catch(ObjectDisposedException ex)
{
Console.WriteLine("Error: " + ex.Message);
}
Console.WriteLine("SendCallback returning");
}
static void Main(string[] args)
{
// can be any server that accepts incoming TCP
// connections
const string TEST_HOST = "localhost";
const int TEST_PORT = 80;
ManualResetEvent send_done =
new ManualResetEvent(false);
try
{
Socket socket = new Socket(
AddressFamily.InterNetwork,
SocketType.Stream,
ProtocolType.Tcp
);
try
{
socket.Connect(TEST_HOST, TEST_PORT);
List<ArraySegment<byte>> buffer_list =
new List<ArraySegment<byte>>();
ArraySegment<byte> segment =
new ArraySegment<byte>(
new byte[] { 0 }
);
buffer_list.Add(segment);
socket.BeginSend(
buffer_list,
SocketFlags.None,
SendCallback,
new SendCallBackArgs(
send_done, socket
)
);
// simulate timing that can cause
// callback and Close to race
Thread.Sleep(250);
}
finally
{
Console.WriteLine("doing work...");
// try to cancel send (or receive or
// whatever asynchronous callback is
// running) because it's time to
// shutdown
Console.WriteLine(
"shutting down subsystem..."
);
try
{
socket.Shutdown(
SocketShutdown.Both
);
Console.WriteLine(
"Shutdown called on socket"
);
}
finally
{
socket.Close();
Console.WriteLine(
"Close called on socket"
);
}
}
}
finally
{
send_done.Close();
Console.WriteLine("Close called on event");
}
// simulate timing that can allow callback to
// proceed (and error)
Thread.Sleep(2000);
Console.WriteLine("end of test");
}
private class SendCallBackArgs
{
public SendCallBackArgs(
ManualResetEvent evt,
Socket socket
)
{
this.evt = evt;
this.socket = socket;
}
private readonly ManualResetEvent evt;
public ManualResetEvent Event
{ get { return evt; } }
private readonly Socket socket;
public Socket Socket
{ get { return socket; } }
}
}
}
Output:
SendCallback entered
doing work...
shutting down subsystem...
Shutdown called on socket
Close called on socket
Close called on event
Error: Safe handle has been closed
SendCallback returning
end of test
Now, while you could simply catch and ignore these exceptions and hope
the rest of the state of any objects involved is okay, that is sloppy,
unpredictable, and bad practice, especially when there are reasonable
ways to do the right thing, such as communicating in a serialized way
the main thread's intent to shutdown and any callbacks' intent to
proceed. (I.e. using an atomic reference count, an event, and some care
in the ordering of operations.)
Peter Duniho
> I'm ignoring any additional comments from people who don't understand
> the common sense of not accessing resources after they've been released,
> but for anyone reasonable who's still interested, here is an example
> that shows that catching ObjectDisposedException in an asynchronous
> callback is not a thread-safe way to determine whether the socket has
> already had Close/Dispose called on it. [...]
Your example has to do with misuse of a completely different object
(ManualResetEvent). It has nothing to do with the discussion at hand.
If that's the kind of sample you provided to Microsoft in your (so-far
still undocumented) bug report, it's no wonder they ignored it.
Show a code example where it's the use of Socket itself that causes a
problem. Creating a new object and then intentionally disposing and
accessing it in a thread-unsafe way doesn't prove anything.
Pete
Matthew
It's inspired from a reference counting technique that's useful when
unloading Plug N Play device drivers. It's a shame such a mechanism is
not part of the classes in the .NET Framework that have asynchronous
callbacks.
Anyone who believes in the orderly shutdown of asynchronous callbacks
can do something like this. (Anyone who does not believe in orderly
shutdown or resource acquisition and release should please stop
developing software.)
(www.developersdex.com seems to make words like `List' and
`ArraySegment' lower case when they shouldn't be below)
using System;
using System.Collections.Generic;
using System.Threading;
using System.Net.Sockets;
namespace Solution
{
class Program
{
static void SendCallback(IAsyncResult ar)
{
Console.WriteLine("SendCallback entered");
SendCallBackArgs args =
(SendCallBackArgs)ar.AsyncState;
try
{
#if true
args.Socket.EndSend(ar);
// simulate timing that can cause callback
// and Close to race
Thread.Sleep(1000);
args.Event.Set();
#else
// simulate timing that can cause callback
// and Close to race
Thread.Sleep(1000);
args.Event.Set();
args.Socket.EndSend(ar);
#endif
}
catch(ObjectDisposedException ex)
{
Console.WriteLine("Error: " + ex.Message);
}
Console.WriteLine("SendCallback returning");
}
static void Main(string[] args)
{
// can be any server that accepts incoming TCP
// connections
const string TEST_HOST = "localhost";
const int TEST_PORT = 80;
ManualResetEvent send_done =
new ManualResetEvent(false);
try
{
SafeSocket socket = new SafeSocket(
socket.Dispose();
}
}
finally
{
send_done.Close();
Console.WriteLine("Close called on event");
}
// simulate timing that can allow callback to
// proceed (and error)
Thread.Sleep(2000);
Console.WriteLine("end of test");
}
private class SendCallBackArgs
{
public SendCallBackArgs(
ManualResetEvent evt,
SafeSocket socket
)
{
this.evt = evt;
this.socket = socket;
}
private readonly ManualResetEvent evt;
public ManualResetEvent Event
{ get { return evt; } }
private readonly SafeSocket socket;
public SafeSocket Socket
{ get { return socket; } }
}
}
// a shutdown mechanism to prevent asynchronous
// callbacks from potentially accessing resources that
// have already been disposed
// for instance, a callback associated with
// System.Threading.Timer may still run after its
// parameterless Dispose has been called, and the same
// is true of the BeginSend method of the Socket class
class AsyncUsage
{
private bool shutting_down = false;
private bool shut_down = false;
private int count = 1;
private ManualResetEvent unused = null;
public AsyncUsage()
{
unused = new ManualResetEvent(false);
}
// to be called at the beginning of some
// asynchronous callback; returns true if usage is
// acquired
public bool Acquire()
{
// attempt to reserve usage
Interlocked.Increment(ref count);
// check if further asynchronous operations
// are prohibited;
// this must occur _after_ the increment
bool acquired = !shutting_down;
if(!acquired)
{
Release();
}
return acquired;
}
// to be called at the end of some asynchronous
// callback
public void Release()
{
int new_count = Interlocked.Decrement(
ref count
);
if(new_count == 0 && !shut_down)
{
shut_down = true;
unused.Set();
}
}
// to be called before any resources used by
// asynchronous callback(s) are disposed
public void FinalWaitRelease()
{
if(!shutting_down)
{
// prevent any new asynchronous operations
// from beginning; this must occur
// _before_ the Release
shutting_down = true;
// wait for all current asynchronous
// operations to finish
Release();
unused.WaitOne();
unused.Close();
unused = null;
}
}
}
// a socket wrapper that ensures its callback(s) will
// do nothing after a successful return from Dispose()
public class SafeSocket : IDisposable
{
private AsyncUsage async_usage = null;
private Socket socket = null;
private AsyncCallback
send_callback,
receive_callback;
private bool disposed = false;
public SafeSocket(Socket a_socket)
{
async_usage = new AsyncUsage();
socket = a_socket;
}
public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}
private void Dispose(bool disposing)
{
if(!this.disposed)
{
if(disposing)
{
// clean up managed resources
async_usage.FinalWaitRelease();
if(socket != null)
{
try
{
socket.Shutdown(
SocketShutdown.Both
);
}
finally
{
socket.Close();
socket = null;
}
}
}
// clean up unmanaged resources (if there
// are any)
disposed = true;
}
}
~SafeSocket()
{
Dispose(false);
}
public void Connect(string host, int port)
{
socket.Connect(host, port);
}
public IAsyncResult BeginSend(
IList<ArraySegment<byte>> buffers,
SocketFlags socketFlags,
AsyncCallback callback,
Object state
)
{
send_callback = callback;
return socket.BeginSend(
buffers,
socketFlags,
SafeSendCallback,
state
);
}
public int EndSend(IAsyncResult asyncResult)
{
return socket.EndSend(asyncResult);
}
public IAsyncResult BeginReceive(
IList<ArraySegment<byte>> buffers,
SocketFlags socketFlags,
AsyncCallback callback,
Object state
)
{
receive_callback = callback;
return socket.BeginReceive(
buffers,
socketFlags,
SafeReceiveCallback,
state
);
}
public int EndReceive(IAsyncResult asyncResult)
{
return socket.EndReceive(asyncResult);
}
private void SafeSendCallback(IAsyncResult ar)
{
if(async_usage.Acquire())
{
try
{
send_callback(ar);
}
finally
{
async_usage.Release();
}
}
}
private void SafeReceiveCallback(IAsyncResult ar)
{
if(async_usage.Acquire())
{
try
{
receive_callback(ar);
}
finally
{
async_usage.Release();
}
}
}
}
}
Output:
SendCallback entered
doing work...
shutting down subsystem...
SendCallback returning
Close called on event
end of test
Matthew
async_usage.FinalWaitRelease();
disposed = true;
}
}
~SafeSocket()
{
Dispose(false);
}
Matthew
if(new_count == 0 && !shut_down)
{
shut_down = true;
unused.Set();
}
should be fixed in these two locations:
private int shut_down = 0;
..
if( new_count == 0 &&
Interlocked.Exchange(ref shut_down, 1) == 0)
{
unused.Set();