Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add the callback of send operation. #458

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add the callback of send operation. #458

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f1e1cee
Add the callback of send operation.
NathanLi Aug 14, 2016
c7b8f8d
Merge remote-tracking branch 'facebook/master'
NathanLi Nov 3, 2016
296c151
Rename SRSendCompleteBlock to SRSendCompletionBlock
NathanLi Nov 3, 2016
f599d0c
Fixed naming problems
NathanLi Nov 3, 2016
ac1c632
Change the call back key for uniqune.
NathanLi Nov 3, 2016
399e25b
Remove useless white space.
NathanLi Nov 3, 2016
00545e5
Remove useless white space.
NathanLi Nov 3, 2016
b313fe8
Improving the method chains and signatures for error:completion: styl…
NathanLi Nov 8, 2016
706623c
Use `[NSValue valueWithRange:dataRange]` as a key of _sendCallbacks.
NathanLi Nov 8, 2016
f630b95
Change the error message.
NathanLi Dec 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions SocketRocket/SRWebSocket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ extern NSString *const SRHTTPResponseErrorKey;

@protocol SRWebSocketDelegate;

typedef void (^SRSendCompleteBlock)(NSError * _Nullable error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Rename to SRSendCompletionBlock.


///--------------------------------------
#pragma mark - SRWebSocket
///--------------------------------------
Expand Down Expand Up @@ -278,6 +280,18 @@ extern NSString *const SRHTTPResponseErrorKey;
*/
- (BOOL)sendString:(NSString *)string error:(NSError **)error NS_SWIFT_NAME(send(string:));

/**
Send a UTF-8 String to the server.

@param string String to send.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Please align String to send with the next parameter description for better readability.

@param complete The call back of send result.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Double space after complete

If an error occurs, this block will invoked with an `NSerror` object cantaining information about the error; Otherwise
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: NSError
Nit: containing
Nit: , otherwise this block will be invoked

this block will invoked with `nil`.

@return `YES` if the string was scheduled to send, otherwise - `NO`.
*/
- (BOOL)sendString:(NSString *)string complete:(SRSendCompleteBlock)complete;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's improve the naming here to the following. Also, will improve the Swift imported method.

- (BOOL)sendString:(NSString *)string 
        completion:(nullable SRSendCompletionBlock)completion NS_SWIFT_NAME(send(string:completion:));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above code block also ensures that everyone can pass nil to the completion argument, since it looks like you are validating it anyway.


/**
Send binary data to the server.

Expand All @@ -302,6 +316,18 @@ extern NSString *const SRHTTPResponseErrorKey;
*/
- (BOOL)sendDataNoCopy:(nullable NSData *)data error:(NSError **)error NS_SWIFT_NAME(send(dataNoCopy:));

/**
Send binary data to the server, without making a defensive copy of it first.

@param data Data to send.
@param complete The call back of send result.
If an error occurs, this block will invoked with an `NSerror` object cantaining information about the error; Otherwise
this block will invoked with `nil`.

@return `YES` if the string was scheduled to send, otherwise - `NO`.
*/
- (BOOL)sendDataNoCopy:(nullable NSData *)data completed:(SRSendCompleteBlock)complete;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above method aka rename this to

- (BOOL)sendDataNoCopy:(nullable NSData *)data 
            completion:(nullable SRSendCompletionBlock)completion NS_SWIFT_NAME(send(dataNoCopy:completion:));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above code block also ensures that everyone can pass nil to the completion argument, since it looks like you are validating it anyway.


/**
Send Ping message to the server with optional data.

Expand Down
152 changes: 121 additions & 31 deletions SocketRocket/SRWebSocket.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,14 @@
NSString *const SRWebSocketErrorDomain = @"SRWebSocketErrorDomain";
NSString *const SRHTTPResponseErrorKey = @"HTTPResponseStatusCode";

@interface SRDataCallback : NSObject
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are constructing this one time only, let's ensure we can't mutate completion later.
You would need to replace the code block with this:

@interface SRDataCallback : NSObject

@property (nonatomic, assign) NSRange range;
@property (nonatomic, copy, readonly) SRSendCompletionBlock completion;

+ (instancetype)new NS_UNAVAILABLE;
- (instancetype)init NS_UNAVAILABLE;
- (instancetype)initWithRange:(NSRange)range 
                   completion:(SRSendCompletionBlock)completion NS_DESIGNATED_INITIALIZER;

@end

@implementation SRDataCallback

- (instancetype)initWithRange:(NSRange)range completion:(SRSendCompletionBlock)completion
{
  self = [super init];

  _range = range;
  _completion = [completion copy];

  return self;
}

@end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above code block also contains the fixes for minor nits, like whitespace, naming as well as proper method attributes (NS_UNAVAILABLE to make sure no one can initialize this class with alloc] init] or new as well as attribute for designated initializer).

@property (nonatomic, assign) NSRange range;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Remove extra space aka make it look like NSRange range;

@property (nonatomic, copy) SRSendCompleteBlock completeBlock;
@end

@implementation SRDataCallback
@end

@interface SRWebSocket () <NSStreamDelegate>

@property (atomic, assign, readwrite) SRReadyState readyState;
Expand Down Expand Up @@ -138,6 +146,9 @@ @implementation SRWebSocket {

// proxy support
SRProxyConnect *_proxyConnect;

NSMutableDictionary<NSNumber *, SRDataCallback *> *_sendCallbacks;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Remove unneeded whitespace.

}

@synthesize readyState = _readyState;
Expand Down Expand Up @@ -179,6 +190,8 @@ - (instancetype)initWithURLRequest:(NSURLRequest *)request protocols:(NSArray<NS
_consumerPool = [[SRIOConsumerPool alloc] init];

_scheduledRunloops = [[NSMutableSet alloc] init];

_sendCallbacks = [[NSMutableDictionary alloc] init];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Either use [NSMutableDictionary dictionary] or even better [NSMutableDictionary new]


return self;
}
Expand Down Expand Up @@ -545,6 +558,13 @@ - (void)_closeWithProtocolError:(NSString *)message;
- (void)_failWithError:(NSError *)error;
{
dispatch_async(_workQueue, ^{
[_sendCallbacks enumerateKeysAndObjectsUsingBlock:^(NSNumber * _Nonnull key, SRDataCallback * _Nonnull obj, BOOL * _Nonnull stop) {
if (obj.completeBlock) {
obj.completeBlock(error);
}
}];
[_sendCallbacks removeAllObjects];

if (self.readyState != SR_CLOSED) {
_failed = YES;
[self.delegateController performDelegateBlock:^(id<SRWebSocketDelegate> _Nullable delegate, SRDelegateAvailableMethods availableMethods) {
Expand All @@ -563,14 +583,27 @@ - (void)_failWithError:(NSError *)error;
});
}

- (void)_writeData:(NSData *)data;
- (void)_writeData:(NSData *)data
{
[self assertOnWorkQueue];
[self _writeData:data complete:NULL];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use nil for completion argument, since blocks are ObjC objects.

}

- (void)_writeData:(NSData *)data complete:(SRSendCompleteBlock)complete {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as public API - please rename to

- (void)_writeData:(NSData *)data completion:(nullable SRSendCompletionBlock)completion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And another nit on top of this:
The opening bracket should be on the next line, per common style of the surrounding code.

[self assertOnWorkQueue];

if (_closeWhenFinishedWriting) {
if (complete) {
complete(SRErrorWithCodeDescription(2134, @"socket is closed"));
}
return;
}


SRDataCallback *record = [[SRDataCallback alloc] init];
record.completeBlock = complete;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This completion can be nil and we are going to waste precious (on iOS at least) CPU cycles for running all this math later.
What do you think if we don't add the record to the _sendCallbacks at all if completion is nil?
This is going to ease the work later as well.

NSUInteger location = dispatch_data_get_size(_outputBuffer);
record.range = NSMakeRange(location, [data length]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Please use dot-syntax for ObjC properties aka data.length instead of [data length].

_sendCallbacks[@([data hash])] = record;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the most important part of this code review:

hash property on NSObject, and as a result NSData is not guaranteed to be unique, meaning that in an unlikely event - you can run into collisions.

You need to change the key here to probably [NSValue valueWIthRange:range] and then to retrieve it - use the NSValue constructor once again.


__block NSData *strongData = data;
dispatch_data_t newData = dispatch_data_create(data.bytes, data.length, nil, ^{
strongData = nil;
Expand All @@ -594,18 +627,32 @@ - (void)send:(nullable id)message

- (BOOL)sendString:(NSString *)string error:(NSError **)error
{
return [self sendString:string error:error complete:NULL];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Use nil for completion argument.

}

- (BOOL)sendString:(NSString *)string complete:(SRSendCompleteBlock)complete {
return [self sendString:string error:NULL complete:complete];
}

- (BOOL)sendString:(NSString *)string error:(NSError **)error complete:(SRSendCompleteBlock)complete {
if (self.readyState != SR_OPEN) {
NSString *message = @"Invalid State: Cannot call `sendString:error:` until connection is open.";
NSError *locialError = SRErrorWithCodeDescription(2134, message);
if (error) {
*error = SRErrorWithCodeDescription(2134, message);
*error = locialError;
}

if (complete) {
complete(locialError);
}

SRDebugLog(message);
return NO;
}

string = [string copy];
dispatch_async(_workQueue, ^{
[self _sendFrameWithOpcode:SROpCodeTextFrame data:[string dataUsingEncoding:NSUTF8StringEncoding]];
[self _sendFrameWithOpcode:SROpCodeTextFrame data:[string dataUsingEncoding:NSUTF8StringEncoding] complete:complete];
});
return YES;
}
Expand All @@ -618,20 +665,33 @@ - (BOOL)sendData:(nullable NSData *)data error:(NSError **)error

- (BOOL)sendDataNoCopy:(nullable NSData *)data error:(NSError **)error
{
return [self sendDataNoCopy:data error:error completed:NULL];
}

- (BOOL)sendDataNoCopy:(nullable NSData *)data completed:(SRSendCompleteBlock)complete {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Opening bracket on the next line.

return [self sendDataNoCopy:data error:NULL completed:complete];
}

- (BOOL)sendDataNoCopy:(nullable NSData *)data error:(NSError **)error completed:(SRSendCompleteBlock)complete {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Opening bracket on the next line.

if (self.readyState != SR_OPEN) {
NSString *message = @"Invalid State: Cannot call `sendDataNoCopy:error:` until connection is open.";
NSError *locialError = SRErrorWithCodeDescription(2134, message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: localError?

if (error) {
*error = SRErrorWithCodeDescription(2134, message);
*error = locialError;
}

if (complete) {
complete(locialError);
}
SRDebugLog(message);
return NO;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Revert white space addition.

dispatch_async(_workQueue, ^{
if (data) {
[self _sendFrameWithOpcode:SROpCodeBinaryFrame data:data];
[self _sendFrameWithOpcode:SROpCodeBinaryFrame data:data complete:complete];
} else {
[self _sendFrameWithOpcode:SROpCodeTextFrame data:nil];
[self _sendFrameWithOpcode:SROpCodeTextFrame data:nil complete:complete];
}
});
return YES;
Expand Down Expand Up @@ -1051,10 +1111,29 @@ - (void)_pumpWriting;
[self _failWithError:error];
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Remove whitespace addition.

_outputBufferOffset += bytesWritten;

NSMutableArray<NSNumber *> *removeKeys = [NSMutableArray array];
[_sendCallbacks enumerateKeysAndObjectsUsingBlock:^(NSNumber * _Nonnull key, SRDataCallback * _Nonnull obj, BOOL * _Nonnull stop) {
if (NSMaxRange(obj.range) <= _outputBufferOffset) {
[removeKeys addObject:key];
if (obj.completeBlock) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can go away once we can ensure that _sendCallbacks only contain objects with non-nil completion.

obj.completeBlock(nil);
}
}


}];
[_sendCallbacks removeObjectsForKeys:removeKeys];

if (_outputBufferOffset > SRDefaultBufferSize() && _outputBufferOffset > dataLength / 2) {
[_sendCallbacks enumerateKeysAndObjectsUsingBlock:^(NSNumber * _Nonnull key, SRDataCallback * _Nonnull obj, BOOL * _Nonnull stop) {
NSRange range = obj.range;
range.location -= _outputBufferOffset;
obj.range = range;
}];

_outputBuffer = dispatch_data_create_subrange(_outputBuffer, _outputBufferOffset, dataLength - _outputBufferOffset);
_outputBufferOffset = 0;
}
Expand Down Expand Up @@ -1314,74 +1393,85 @@ -(void)_pumpScanner;

static const size_t SRFrameHeaderOverhead = 32;

- (void)_sendFrameWithOpcode:(SROpCode)opCode data:(NSData *)data
{
- (void)_sendFrameWithOpcode:(SROpCode)opCode data:(NSData *)data complete:(SRSendCompleteBlock)complete {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Move the opening bracket to the next line.

[self assertOnWorkQueue];

if (!data) {
if (complete) {
complete(nil);
}
return;
}

size_t payloadLength = data.length;

NSMutableData *frameData = [[NSMutableData alloc] initWithLength:payloadLength + SRFrameHeaderOverhead];
if (!frameData) {
[self closeWithCode:SRStatusCodeMessageTooBig reason:@"Message too big"];
NSString *reason = @"Message too big";
[self closeWithCode:SRStatusCodeMessageTooBig reason:reason];
if (complete) {
complete(SRErrorWithCodeDescription(SRStatusCodeMessageTooBig, reason));
}
return;
}
uint8_t *frameBuffer = (uint8_t *)frameData.mutableBytes;

// set fin
frameBuffer[0] = SRFinMask | opCode;

// set the mask and header
frameBuffer[1] |= SRMaskMask;

size_t frameBufferSize = 2;

if (payloadLength < 126) {
frameBuffer[1] |= payloadLength;
} else {
uint64_t declaredPayloadLength = 0;
size_t declaredPayloadLengthSize = 0;

if (payloadLength <= UINT16_MAX) {
frameBuffer[1] |= 126;

declaredPayloadLength = CFSwapInt16BigToHost((uint16_t)payloadLength);
declaredPayloadLengthSize = sizeof(uint16_t);
} else {
frameBuffer[1] |= 127;

declaredPayloadLength = CFSwapInt64BigToHost((uint64_t)payloadLength);
declaredPayloadLengthSize = sizeof(uint64_t);
}

memcpy((frameBuffer + frameBufferSize), &declaredPayloadLength, declaredPayloadLengthSize);
frameBufferSize += declaredPayloadLengthSize;
}

const uint8_t *unmaskedPayloadBuffer = (uint8_t *)data.bytes;
uint8_t *maskKey = frameBuffer + frameBufferSize;

size_t randomBytesSize = sizeof(uint32_t);
int result = SecRandomCopyBytes(kSecRandomDefault, randomBytesSize, maskKey);
if (result != 0) {
//TODO: (nlutsenko) Check if there was an error.
}
frameBufferSize += randomBytesSize;

// Copy and unmask the buffer
uint8_t *frameBufferPayloadPointer = frameBuffer + frameBufferSize;

memcpy(frameBufferPayloadPointer, unmaskedPayloadBuffer, payloadLength);
SRMaskBytesSIMD(frameBufferPayloadPointer, payloadLength, maskKey);
frameBufferSize += payloadLength;

assert(frameBufferSize <= frameData.length);
frameData.length = frameBufferSize;

[self _writeData:frameData complete:complete];
}

[self _writeData:frameData];
- (void)_sendFrameWithOpcode:(SROpCode)opCode data:(NSData *)data
{
[self _sendFrameWithOpcode:opCode data:data complete:NULL];
}

- (void)stream:(NSStream *)aStream handleEvent:(NSStreamEvent)eventCode
Expand Down