From 238ad7819ffcd650692baff57d15b577503fd448 Mon Sep 17 00:00:00 2001 From: Jorge Canizales Date: Fri, 7 Aug 2015 23:11:29 -0700 Subject: Eliminate race in GRPCCall’s operation of the requests writer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/objective-c/GRPCClient/GRPCCall.m | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) (limited to 'src/objective-c/GRPCClient') diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index 5f7d74bca8..16abd0fadf 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -74,6 +74,13 @@ NSString * const kGRPCStatusMetadataKey = @"io.grpc.StatusMetadataKey"; // all. This wrapper over our actual writeable ensures thread-safety and // correct ordering. GRXConcurrentWriteable *_responseWriteable; + + // The network thread wants the requestWriter to resume (when the server is ready for more input), + // or to stop (on errors), concurrently with user threads that want to start it, pause it or stop + // it. Because a writer isn't thread-safe, we'll synchronize those operations on it. + // We don't use a dispatch queue for that purpose, because the writer can call writeValue: or + // writesFinishedWithError: on this GRPCCall as part of those operations. We want to be able to + // pause the writer immediately on writeValue:, so we need our locking to be recursive. GRXWriter *_requestWriter; // To create a retain cycle when a call is started, up until it finishes. See @@ -139,8 +146,10 @@ NSString * const kGRPCStatusMetadataKey = @"io.grpc.StatusMetadataKey"; _self = nil; // If there were still request messages coming, stop them. - _requestWriter.state = GRXWriterStateFinished; - _requestWriter = nil; + @synchronized(_requestWriter) { + _requestWriter.state = GRXWriterStateFinished; + _requestWriter = nil; + } if (errorOrNil) { [_responseWriteable cancelWithError:errorOrNil]; @@ -240,12 +249,14 @@ NSString * const kGRPCStatusMetadataKey = @"io.grpc.StatusMetadataKey"; // Resume the request writer. GRPCCall *strongSelf = weakSelf; if (strongSelf) { - strongSelf->_requestWriter.state = GRXWriterStateStarted; + @synchronized(_requestWriter) { + strongSelf->_requestWriter.state = GRXWriterStateStarted; + } } }; - [_wrappedCall startBatchWithOperations:@[[[GRPCOpSendMessage alloc] - initWithMessage:message - handler:resumingHandler]] errorHandler:errorHandler]; + [_wrappedCall startBatchWithOperations:@[[[GRPCOpSendMessage alloc] initWithMessage:message + handler:resumingHandler]] + errorHandler:errorHandler]; } - (void)writeValue:(id)value { @@ -253,7 +264,9 @@ NSString * const kGRPCStatusMetadataKey = @"io.grpc.StatusMetadataKey"; // Pause the input and only resume it when the C layer notifies us that writes // can proceed. - _requestWriter.state = GRXWriterStatePaused; + @synchronized(_requestWriter) { + _requestWriter.state = GRXWriterStatePaused; + } __weak GRPCCall *weakSelf = self; dispatch_async(_callQueue, ^{ @@ -273,7 +286,9 @@ NSString * const kGRPCStatusMetadataKey = @"io.grpc.StatusMetadataKey"; } - (void)writesFinishedWithError:(NSError *)errorOrNil { - _requestWriter = nil; + @synchronized(_requestWriter) { + _requestWriter = nil; + } if (errorOrNil) { [self cancel]; } else { @@ -327,7 +342,9 @@ NSString * const kGRPCStatusMetadataKey = @"io.grpc.StatusMetadataKey"; } }]; // Now that the RPC has been initiated, request writes can start. - [_requestWriter startWithWriteable:self]; + @synchronized(_requestWriter) { + [_requestWriter startWithWriteable:self]; + } } #pragma mark GRXWriter implementation -- cgit v1.2.3 From eb87b4653a40ee9e6226687ec3f9306c0b9a89df Mon Sep 17 00:00:00 2001 From: Jorge Canizales Date: Sat, 8 Aug 2015 16:16:43 -0700 Subject: Rename super-confusing ivar _self -> _retainSelf --- src/objective-c/GRPCClient/GRPCCall.m | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'src/objective-c/GRPCClient') diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index 16abd0fadf..405f0335e7 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -84,8 +84,10 @@ NSString * const kGRPCStatusMetadataKey = @"io.grpc.StatusMetadataKey"; GRXWriter *_requestWriter; // To create a retain cycle when a call is started, up until it finishes. See - // |startWithWriteable:| and |finishWithError:|. - GRPCCall *_self; + // |startWithWriteable:| and |finishWithError:|. This saves users from having to retain a + // reference to the call object if all they're interested in is the handler being executed when + // the response arrives. + GRPCCall *_retainSelf; NSMutableDictionary *_requestMetadata; NSMutableDictionary *_responseMetadata; @@ -143,7 +145,7 @@ NSString * const kGRPCStatusMetadataKey = @"io.grpc.StatusMetadataKey"; - (void)finishWithError:(NSError *)errorOrNil { // If the call isn't retained anywhere else, it can be deallocated now. - _self = nil; + _retainSelf = nil; // If there were still request messages coming, stop them. @synchronized(_requestWriter) { @@ -355,7 +357,7 @@ NSString * const kGRPCStatusMetadataKey = @"io.grpc.StatusMetadataKey"; // before being autoreleased). // Care is taken not to retain self strongly in any of the blocks used in this implementation, so // that the life of the instance is determined by this retain cycle. - _self = self; + _retainSelf = self; _responseWriteable = [[GRXConcurrentWriteable alloc] initWithWriteable:writeable]; [self sendHeaders:_requestMetadata]; -- cgit v1.2.3 From 297ed7bd81ba5a8e607f278713fb0facc9475b91 Mon Sep 17 00:00:00 2001 From: Jorge Canizales Date: Sat, 8 Aug 2015 16:29:52 -0700 Subject: Don’t set the request writer to nil, as @synchr(nil) is undefined behavior. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also clarify in GRXWriter.h that the writeable is released whenever the writer finishes. --- src/objective-c/GRPCClient/GRPCCall.m | 4 --- src/objective-c/RxLibrary/GRXWriter.h | 52 +++++++++++++++-------------------- 2 files changed, 22 insertions(+), 34 deletions(-) (limited to 'src/objective-c/GRPCClient') diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index 405f0335e7..6836d34394 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -150,7 +150,6 @@ NSString * const kGRPCStatusMetadataKey = @"io.grpc.StatusMetadataKey"; // If there were still request messages coming, stop them. @synchronized(_requestWriter) { _requestWriter.state = GRXWriterStateFinished; - _requestWriter = nil; } if (errorOrNil) { @@ -288,9 +287,6 @@ NSString * const kGRPCStatusMetadataKey = @"io.grpc.StatusMetadataKey"; } - (void)writesFinishedWithError:(NSError *)errorOrNil { - @synchronized(_requestWriter) { - _requestWriter = nil; - } if (errorOrNil) { [self cancel]; } else { diff --git a/src/objective-c/RxLibrary/GRXWriter.h b/src/objective-c/RxLibrary/GRXWriter.h index 65c8806d75..b1c994aa38 100644 --- a/src/objective-c/RxLibrary/GRXWriter.h +++ b/src/objective-c/RxLibrary/GRXWriter.h @@ -35,33 +35,28 @@ #import "GRXWriteable.h" +// States of a writer. typedef NS_ENUM(NSInteger, GRXWriterState) { - // The writer has not yet been given a writeable to which it can push its - // values. To have an writer transition to the Started state, send it a - // startWithWriteable: message. + // The writer has not yet been given a writeable to which it can push its values. To have a writer + // transition to the Started state, send it a startWithWriteable: message. // - // An writer's state cannot be manually set to this value. + // A writer's state cannot be manually set to this value. GRXWriterStateNotStarted, // The writer might push values to the writeable at any moment. GRXWriterStateStarted, - // The writer is temporarily paused, and won't send any more values to the - // writeable unless its state is set back to Started. The writer might still - // transition to the Finished state at any moment, and is allowed to send - // writesFinishedWithError: to its writeable. - // - // Not all implementations of writer have to support pausing, and thus - // trying to set an writer's state to this value might have no effect. + // The writer is temporarily paused, and won't send any more values to the writeable unless its + // state is set back to Started. The writer might still transition to the Finished state at any + // moment, and is allowed to send writesFinishedWithError: to its writeable. GRXWriterStatePaused, // The writer has released its writeable and won't interact with it anymore. // - // One seldomly wants to set an writer's state to this value, as its - // writeable isn't notified with a writesFinishedWithError: message. Instead, sending - // finishWithError: to the writer will make it notify the writeable and then - // transition to this state. + // One seldomly wants to set a writer's state to this value, as its writeable isn't notified with + // a writesFinishedWithError: message. Instead, sending finishWithError: to the writer will make + // it notify the writeable and then transition to this state. GRXWriterStateFinished }; @@ -88,28 +83,25 @@ typedef NS_ENUM(NSInteger, GRXWriterState) { // GRXWriter. I.e., conforming classes aren't required to be thread-safe. @interface GRXWriter : NSObject -// This property can be used to query the current state of the writer, which -// determines how it might currently use its writeable. Some state transitions can -// be triggered by setting this property to the corresponding value, and that's -// useful for advanced use cases like pausing an writer. For more details, -// see the documentation of the enum. +// This property can be used to query the current state of the writer, which determines how it might +// currently use its writeable. Some state transitions can be triggered by setting this property to +// the corresponding value, and that's useful for advanced use cases like pausing an writer. For +// more details, see the documentation of the enum further down. @property(nonatomic) GRXWriterState state; -// Start sending messages to the writeable. Messages may be sent before the method -// returns, or they may be sent later in the future. See GRXWriteable.h for the -// different messages a writeable can receive. +// Transition to the Started state, and start sending messages to the writeable (a reference to it +// is retained). Messages to the writeable may be sent before the method returns, or they may be +// sent later in the future. See GRXWriteable.h for the different messages a writeable can receive. // -// If this writer draws its values from an external source (e.g. from the -// filesystem or from a server), calling this method will commonly trigger side -// effects (like network connections). +// If this writer draws its values from an external source (e.g. from the filesystem or from a +// server), calling this method will commonly trigger side effects (like network connections). // // This method might only be called on writers in the NotStarted state. - (void)startWithWriteable:(id)writeable; -// Send writesFinishedWithError:errorOrNil immediately to the writeable, and don't send -// any more messages to it. +// Send writesFinishedWithError:errorOrNil to the writeable. Then release the reference to it and +// transition to the Finished state. // -// This method might only be called on writers in the Started or Paused -// state. +// This method might only be called on writers in the Started or Paused state. - (void)finishWithError:(NSError *)errorOrNil; @end -- cgit v1.2.3 From 578ab166adc49f3d8c1fccdb1f5a364e7011c8ec Mon Sep 17 00:00:00 2001 From: Jorge Canizales Date: Sat, 8 Aug 2015 17:11:43 -0700 Subject: Don’t retain self here! MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/objective-c/GRPCClient/GRPCCall.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/objective-c/GRPCClient') diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index 6836d34394..0f4c811ce4 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -250,7 +250,7 @@ NSString * const kGRPCStatusMetadataKey = @"io.grpc.StatusMetadataKey"; // Resume the request writer. GRPCCall *strongSelf = weakSelf; if (strongSelf) { - @synchronized(_requestWriter) { + @synchronized(strongSelf->_requestWriter) { strongSelf->_requestWriter.state = GRXWriterStateStarted; } } -- cgit v1.2.3