Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

Bug fix for crash - reported via CocoaAsyncSocket project (issue #83)

  • Loading branch information...
commit 0f4e3c244df0345511ade45af3732328980811ed 1 parent 5d687ec
Robbie Hanson robbiehanson authored

Showing 1 changed file with 53 additions and 6 deletions. Show diff stats Hide diff stats

  1. +53 6 HTTPConnection.m
59 HTTPConnection.m
@@ -581,7 +581,9 @@ - (void)stop
581 581 dispatch_async(connectionQueue, ^{
582 582 NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
583 583
584   - [self die];
  584 + // Disconnect the socket.
  585 + // The socketDidDisconnect delegate method will handle everything else.
  586 + [asyncSocket disconnect];
585 587
586 588 [pool release];
587 589 });
@@ -930,10 +932,46 @@ - (void)replyToHTTPRequest
930 932
931 933 [[config server] addWebSocket:ws];
932 934
933   - [asyncSocket release];
934   - asyncSocket = nil;
935   -
936   - [self die];
  935 + // The WebSocket should now be the delegate of the underlying socket.
  936 + // But gracefully handle the situation if it forgot.
  937 + if ([asyncSocket delegate] == self)
  938 + {
  939 + HTTPLogWarn(@"%@[%p]: WebSocket forgot to set itself as socket delegate", THIS_FILE, self);
  940 +
  941 + // Disconnect the socket.
  942 + // The socketDidDisconnect delegate method will handle everything else.
  943 + [asyncSocket disconnect];
  944 + }
  945 + else
  946 + {
  947 + // The WebSocket is using the socket,
  948 + // so make sure we don't disconnect it in the dealloc method.
  949 + [asyncSocket release];
  950 + asyncSocket = nil;
  951 +
  952 + [self die];
  953 +
  954 + // Note: There is a timing issue here that should be pointed out.
  955 + //
  956 + // A bug that existed in previous versions happend like so:
  957 + // - We invoked [self die]
  958 + // - This caused us to get released, and our dealloc method to start executing
  959 + // - Meanwhile, AsyncSocket noticed a disconnect, and began to dispatch a socketDidDisconnect at us
  960 + // - The dealloc method finishes execution, and our instance gets freed
  961 + // - The socketDidDisconnect gets run, and a crash occurs
  962 + //
  963 + // So the issue we want to avoid is releasing ourself when there is a possibility
  964 + // that AsyncSocket might be gearing up to queue a socketDidDisconnect for us.
  965 + //
  966 + // In this particular situation notice that we invoke [asyncSocket delegate].
  967 + // This method is synchronous concerning AsyncSocket's internal socketQueue.
  968 + // Which means we can be sure, when it returns, that AsyncSocket has already
  969 + // queued any delegate methods for us if it was going to.
  970 + // And if the delegate methods are queued, then we've been properly retained.
  971 + // Meaning we won't get released / dealloc'd until the delegate method has finished executing.
  972 + //
  973 + // In this rare situation, the die method will get invoked twice.
  974 + }
937 975 }
938 976
939 977 return;
@@ -2223,7 +2261,10 @@ - (void)socket:(GCDAsyncSocket *)sock didWriteDataWithTag:(long)tag
2223 2261
2224 2262 if ([self shouldDie])
2225 2263 {
2226   - [self die];
  2264 + // The only time we should invoke [self die] is from socketDidDisconnect,
  2265 + // or if the socket gets taken over by someone else like a WebSocket.
  2266 +
  2267 + [asyncSocket disconnect];
2227 2268 }
2228 2269 else
2229 2270 {
@@ -2248,6 +2289,9 @@ - (void)socketDidDisconnect:(GCDAsyncSocket *)sock withError:(NSError *)err;
2248 2289 {
2249 2290 HTTPLogTrace();
2250 2291
  2292 + [asyncSocket release];
  2293 + asyncSocket = nil;
  2294 +
2251 2295 [self die];
2252 2296 }
2253 2297
@@ -2388,6 +2432,9 @@ - (void)die
2388 2432
2389 2433 // Override me if you want to perform any custom actions when a connection is closed.
2390 2434 // Then call [super die] when you're done.
  2435 + //
  2436 + // Important: There is a rare timing condition where this method might get invoked twice.
  2437 + // If you override this method, you should be prepared for this situation.
2391 2438
2392 2439 // Inform the http response that we're done
2393 2440 if ([httpResponse respondsToSelector:@selector(connectionDidClose)])

0 comments on commit 0f4e3c2

Please sign in to comment.
Something went wrong with that request. Please try again.