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

Cronet fixes #20464

Merged
merged 1 commit into from
Oct 4, 2019
Merged

Cronet fixes #20464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 13 additions & 8 deletions src/core/ext/transport/cronet/transport/cronet_transport.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1348,21 +1348,26 @@ static enum e_op_result execute_stream_op(struct op_and_state* oas) {
stream_state->cancel_error =
GRPC_ERROR_REF(stream_op->payload->cancel_stream.cancel_error);
}
} else if (stream_op->on_complete &&
op_can_be_run(stream_op, s, &oas->state, OP_ON_COMPLETE)) {
} else if (op_can_be_run(stream_op, s, &oas->state, OP_ON_COMPLETE)) {
CRONET_LOG(GPR_DEBUG, "running: %p OP_ON_COMPLETE", oas);
if (stream_state->state_op_done[OP_CANCEL_ERROR]) {
GRPC_CLOSURE_SCHED(stream_op->on_complete,
GRPC_ERROR_REF(stream_state->cancel_error));
if (stream_op->on_complete) {
GRPC_CLOSURE_SCHED(stream_op->on_complete,
GRPC_ERROR_REF(stream_state->cancel_error));
}
} else if (stream_state->state_callback_received[OP_FAILED]) {
GRPC_CLOSURE_SCHED(
stream_op->on_complete,
make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."));
if (stream_op->on_complete) {
GRPC_CLOSURE_SCHED(
stream_op->on_complete,
make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."));
}
} else {
/* All actions in this stream_op are complete. Call the on_complete
* callback
*/
GRPC_CLOSURE_SCHED(stream_op->on_complete, GRPC_ERROR_NONE);
if (stream_op->on_complete) {
GRPC_CLOSURE_SCHED(stream_op->on_complete, GRPC_ERROR_NONE);
}
}
oas->state.state_op_done[OP_ON_COMPLETE] = true;
oas->done = true;
Expand Down
3 changes: 2 additions & 1 deletion src/objective-c/tests/ConfigureCronet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* limitations under the License.
*
*/
#include <stdbool.h>

#ifdef __cplusplus
extern "C" {
Expand All @@ -23,7 +24,7 @@ extern "C" {
/**
* Enable Cronet for once.
*/
void configureCronet(void);
void configureCronet(bool enable_netlog);

#ifdef __cplusplus
}
Expand Down
6 changes: 4 additions & 2 deletions src/objective-c/tests/ConfigureCronet.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
#import "ConfigureCronet.h"
#import <Cronet/Cronet.h>

void configureCronet(void) {
void configureCronet(bool enable_netlog) {
static dispatch_once_t configureCronet;
dispatch_once(&configureCronet, ^{
NSLog(@"configureCronet()");
Expand All @@ -29,7 +29,9 @@ void configureCronet(void) {
NSURL *url = [[[NSFileManager defaultManager] URLsForDirectory:NSDocumentDirectory
inDomains:NSUserDomainMask] lastObject];
NSLog(@"Documents directory: %@", url);
if (enable_netlog) {
[Cronet startNetLogToFile:@"cronet_netlog.json" logBytes:YES];
}
[Cronet start];
[Cronet startNetLogToFile:@"cronet_netlog.json" logBytes:YES];
Copy link
Member

Choose a reason for hiding this comment

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

Instead of removing it, maybe make this configurable? Sometimes netlog is useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

});
}
2 changes: 1 addition & 1 deletion src/objective-c/tests/CronetTests/CoreCronetEnd2EndTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ + (void)setUp {

grpc_init();

configureCronet();
configureCronet(/*enable_netlog=*/false);
}

// The tearDown() function is run after all test cases finish running
Expand Down
2 changes: 1 addition & 1 deletion src/objective-c/tests/CronetTests/CronetUnitTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ + (void)setUp {
grpc_test_init(1, argv);

grpc_init();
configureCronet();
configureCronet(/*enable_netlog=*/false);
init_ssl();
}

Expand Down
2 changes: 1 addition & 1 deletion src/objective-c/tests/CronetTests/InteropTestsRemoteWithCronet.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ @interface InteropTestsRemoteWithCronet : InteropTests
@implementation InteropTestsRemoteWithCronet

+ (void)setUp {
configureCronet();
configureCronet(/*enable_netlog=*/false);
[GRPCCall useCronetWithEngine:[Cronet getGlobalEngine]];

[super setUp];
Expand Down
2 changes: 1 addition & 1 deletion src/objective-c/tests/InteropTests/InteropTestsMultipleChannels.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ - (void)setUp {
self.continueAfterFailure = NO;

_remoteService = [RMTTestService serviceWithHost:kRemoteSSLHost callOptions:nil];
configureCronet();
configureCronet(/*enable_netlog=*/false);

// Default stack with remote host
GRPCMutableCallOptions *options = [[GRPCMutableCallOptions alloc] init];
Expand Down
38 changes: 32 additions & 6 deletions src/objective-c/tests/PerfTests/PerfTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,8 @@ - (void)pingPongV2APIWithRequest:(RMTStreamingOutputCallRequest *)request
messageCallback:^(id message) {
int indexCopy;
@synchronized(self) {
index += 1;
indexCopy = index;
index += 1;
}
if (indexCopy < numMessages) {
[call writeMessage:request];
Expand Down Expand Up @@ -181,7 +181,7 @@ - (void)testPingPongRPCWithV2API {
[self pingPongV2APIWithRequest:request numMessages:1000 options:options];

[self measureBlock:^{
[self pingPongV2APIWithRequest:request numMessages:10000 options:options];
[self pingPongV2APIWithRequest:request numMessages:1000 options:options];
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain a bit why we are reducing the number? Maybe in the PR description. For record.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to reduce test run time. Takes too long with 10k messages, and we don't really get any additional benefit with more messages. I've updated the PR description.

}];
}

Expand All @@ -198,7 +198,7 @@ - (void)testPingPongRPCWithFlowControl {
[self pingPongV2APIWithRequest:request numMessages:1000 options:options];

[self measureBlock:^{
[self pingPongV2APIWithRequest:request numMessages:10000 options:options];
[self pingPongV2APIWithRequest:request numMessages:1000 options:options];
}];
}

Expand All @@ -215,7 +215,7 @@ - (void)testPingPongRPCWithInterceptor {
[self pingPongV2APIWithRequest:request numMessages:1000 options:options];

[self measureBlock:^{
[self pingPongV2APIWithRequest:request numMessages:10000 options:options];
[self pingPongV2APIWithRequest:request numMessages:1000 options:options];
}];
}

Expand Down Expand Up @@ -254,7 +254,7 @@ - (void)testPingPongRPCWithV1API {
id request = [RMTStreamingOutputCallRequest messageWithPayloadSize:@1 requestedResponseSize:@1];
[self pingPongV1APIWithRequest:request numMessages:1000];
[self measureBlock:^{
[self pingPongV1APIWithRequest:request numMessages:10000];
[self pingPongV1APIWithRequest:request numMessages:1000];
}];
}

Expand Down Expand Up @@ -298,7 +298,7 @@ - (void)unaryRPCsWithServices:(NSArray<RMTTestService *> *)services
[self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil];
}

- (void)testUnaryRPC {
- (void)test1MBUnaryRPC {
// Workaround Apple CFStream bug
if ([self isUsingCFStream]) {
return;
Expand Down Expand Up @@ -329,6 +329,32 @@ - (void)testUnaryRPC {
}];
}

- (void)test1KBUnaryRPC {
RMTSimpleRequest *request = [RMTSimpleRequest message];
request.responseSize = 1024;
request.payload.body = [NSMutableData dataWithLength:1024];

GRPCMutableCallOptions *options = [[GRPCMutableCallOptions alloc] init];
options.transport = [[self class] transport];
options.PEMRootCertificates = [[self class] PEMRootCertificates];
options.hostNameOverride = [[self class] hostNameOverride];

// warm up
[self unaryRPCsWithServices:@[ self->_service ]
request:request
callsPerService:1000
maxOutstandingCalls:100
callOptions:options];

[self measureBlock:^{
[self unaryRPCsWithServices:@[ self->_service ]
request:request
callsPerService:1000
maxOutstandingCalls:100
callOptions:options];
}];
}

- (void)testMultipleChannels {
NSString *port = [[[self class] host] componentsSeparatedByString:@":"][1];
int kNumAddrs = 10;
Expand Down
2 changes: 1 addition & 1 deletion src/objective-c/tests/PerfTests/PerfTestsCronet.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ @interface PerfTestsCronet : PerfTests
@implementation PerfTestsCronet

+ (void)setUp {
configureCronet();
configureCronet(/*enable_netlog=*/false);
[GRPCCall useCronetWithEngine:[Cronet getGlobalEngine]];

[super setUp];
Expand Down