This week's improvements #184

MrMage · 2018-03-12T12:13:22Z

As discussed via Hangouts with @timburks, I'm going to gather all my changes for this week in this PR, then have them merged at once.

Notable changes:

Add a script to fix the project's indentation settings. Requires the CocodaPods 'xcodeproj' gem.
Use the system-default zlib module instead of a vendored one.
Make sure that the caller gets notified of errors in client calls (and add tests for that).
Fix the SSL-enabled tests.
Several fixes to gRPCTests to make them less flaky (before, they would still flake out from time to time on my machine).
Make the syntax of calling send and receive homogenous across all stream-sending/receiving client and server calls.
Add tests to ensure that timeouts are handled as expected, both client- and server-side. I'd also like to introduce a server-enforced timeout at some point, but that has more time.
Make sure that the client is notified about server errors (and add tests for that).
In line with the above, homogenize the interface of closing server sessions and sending results. Unfortunately, that means that you now have to call e.g. session.sendAndClose(response: response, status: .ok, completion: .nil) instead of just session.sendAndClose(response), because methods in protocols can't have default arguments. If you prefer, I could add another method to the protocols for the "default" case (status OK, no completion handler) in lieu of default arguments.

FYI, the increase in lines is almost exclusively due to tests.
Also, I tried to keep the number of commits reasonable, but there is just a lot of stuff in this PR.

Requires the CocodaPods 'xcodeproj' gem.

…ally usable in tests.

SebastianThiebaud · 2018-03-14T01:12:15Z

Nice! I've been struggling to switch indentation settings every time I switch projects. I really hope Xcode projects could embed/store a custom indentation configuration.

MrMage · 2018-03-14T09:09:49Z

They can, but swift generate-xcodeproj overrides that when regenerating the project :-/

The expected behavior is now for the client call's completion handler to be called when the whole call is completed. You can check that completion handler's `statusCode` for the call's result. The intermediate send/receive call completion handlers will also get called with an error, but that error's status code is most likely to be `.unknown` (because send/receive calls don't receive the final status from the server). In addition, `CallResult` now also includes the operation group's success flag, but that flag is much less important now. This includes: - Make most completion handlers non-throwing (their errors were for the most part discarded, anyway) - Extract Receive-Streaming into a common protocol - Changing many completion handler interfaces to be called with a `CallResult` instead of `Data?` and `ResultOrRPCError<ReceivedType?>` instead of `(ReceivedType, Error)` - Not throwing on end of stream, but passing `nil` to the completion handler instead - Making `ServerSessionUnary` always return an error when one of its calls fail - Adding test to ensure the proper error behavior.

…g` protocol.

- avoid starting the server in an asynchronous block (as that can lead to it being shut down too early) - make sure to close bidirectional calls - wait on _all_ `receiveMessage` calls in `handleBiDiStream`. Previously, we would not wait for the first `receiveMessage` call, which could lead to race conditions.

…ers could get called twice, as well as an unsynchronized access to `CompletionQueue.operationGroups` that should be synchronized.

…dentation settings when running `make`.

…g `Call` equivalents as possible, so that only server-specific methods are left in `Handler`.

…t `catch`.

… receiving SHUTDOWN still have their completion handlers called.

…I had forgotten that sending initial metadata is required).

…nectionFailureTests".

MrMage · 2018-03-16T19:16:58Z

This should be ready to review now. Will look into the CI failures soon.

…thods actually *move* the metadata (so a second call to these methods would previously always return empty metadata objects).

…acOS.

MrMage · 2018-03-17T12:11:59Z

Closing and re-opening to measure the effects of the dependency cache...

…, for faster CI builds. (Also separate the dependency resolution and main build steps into separate steps, so we can time each one individually.)

We need to return status OK here, as it seems like the server might never send out the last few messages once it has been asked to send a non-OK status. Alternatively, we could send a non-OK status here, but then we would need to sleep for a few milliseconds before sending the non-OK status.

…r code. These tests don't really test the logic of the SwiftGRPC library, but are meant as an example of how one would go about testing their own client/server code that relies on SwiftGRPC.

…tus without a result.

Also changes a few method signatures where passing a `CallResult` does not make sense.

timburks · 2018-03-17T19:20:34Z

Makefile

@@ -8,6 +8,8 @@ all:

 project:
 	swift package generate-xcodeproj
+	# Optional: set the generated project's indentation settings.


Let's add a comment here to explain how to install the gem. I think it's safe to assume people will have ruby installed, but sudo gem install xcodeproj might not be obvious to non-rubyists.

timburks · 2018-03-17T19:24:44Z

Package.swift

@@ -23,15 +23,15 @@ let package = Package(
    .library(name: "SwiftGRPC", targets: ["SwiftGRPC"]),
  ],
  dependencies: [
-    .package(url: "https://github.com/Zewo/zlib.git", from: "0.4.0"),
+    .package(url: "https://github.com/apple/swift-nio-zlib-support.git", from: "1.0.0"),


I'm not hugely-confident that this project won't move, but since the zewo version was deprecated, I'm ok with this.

timburks

Do we have tests that verify this now?
"Ensure that any operation groups enqueued on a completion queue after receiving SHUTDOWN still have their completion handlers called"

timburks · 2018-03-17T19:42:42Z

Thanks for all these improvements! See a few minor comments above, but overall it looks great.

MrMage added 2 commits March 13, 2018 10:40

Add a script to fix the project's indentation settings.

5665276

Requires the CocodaPods 'xcodeproj' gem.

Add a public initializer for ServerSessionTestStub, to make it actu…

2f93962

…ally usable in tests.

MrMage force-pushed the more-improvements branch from 3c87e6b to 2f93962 Compare March 13, 2018 09:41

Use Apple's "fake" zlib package instead of Zewo's real zlib code.

263e66b

MrMage force-pushed the more-improvements branch from ca0d255 to ee99455 Compare March 15, 2018 15:09

MrMage force-pushed the more-improvements branch from ee99455 to ac7979d Compare March 15, 2018 15:11

MrMage added 14 commits March 15, 2018 16:27

Also extract streaming sending methods into a dedicated `StreamSendin…

be3fae4

…g` protocol.

Fix the SSL-enabled tests.

cacd605

Consolidate a few more calls into StreamSending.

9916aea

Replace a few semaphores with call.messageQueueEmpty.wait().

730699d

Fix a potential race condition where operation group completion handl…

17b32f5

…ers could get called twice, as well as an unsynchronized access to `CompletionQueue.operationGroups` that should be synchronized.

Tweak the visibility of Handler.call.

800cea1

Avoid displaying the comment about setting the generated project's in…

b5e7c19

…dentation settings when running `make`.

Replace some magic numbers with more understandable constants.

8b8e6c8

Cleanup: Replace as many methods on Handler with their correspondin…

ab21b2c

…g `Call` equivalents as possible, so that only server-specific methods are left in `Handler`.

Formatting: Replace several occurrences of catch (let ...) with jus…

f814a34

…t `catch`.

Avoid an extra dispatch in Call.sendWithoutBlocking.

90601ab

Ensure that any operation groups enqueued on a completion queue after…

d3afc45

… receiving SHUTDOWN still have their completion handlers called.

Fix how the server handles unknown methods (broken by me in ac7979d, …

c1b54e6

…I had forgotten that sending initial metadata is required).

MrMage force-pushed the more-improvements branch 2 times, most recently from e786079 to d25d0c5 Compare March 16, 2018 15:08

Add tests to ensure that server-side timeouts are handled as expected.

2ebe048

MrMage force-pushed the more-improvements branch 2 times, most recently from 05b7633 to ad6973c Compare March 16, 2018 15:28

Also add client-timeout tests and rename "ErrorHandlingTests" to "Con…

0985a47

…nectionFailureTests".

MrMage force-pushed the more-improvements branch from ad6973c to 0985a47 Compare March 16, 2018 15:37

MrMage added 3 commits March 17, 2018 12:03

Fix two compiler warnings.

2313a99

Make OperationGroup cache its ...Metadata properties, as those me…

9128b60

…thods actually *move* the metadata (so a second call to these methods would previously always return empty metadata objects).

Make the test expectations on receive consistent across Linux and m…

c962004

…acOS.

MrMage force-pushed the more-improvements branch from 91d7d0e to c962004 Compare March 17, 2018 11:54

Fix a minor error in the Travis build script.

7935aeb

MrMage closed this Mar 17, 2018

MrMage reopened this Mar 17, 2018

MrMage force-pushed the more-improvements branch 2 times, most recently from e0026f4 to bd29ef6 Compare March 17, 2018 12:24

Try making Travis cache the SwiftPM dependencies and APT repositories…

9c54d0a

…, for faster CI builds. (Also separate the dependency resolution and main build steps into separate steps, so we can time each one individually.)

MrMage force-pushed the more-improvements branch from bd29ef6 to 9c54d0a Compare March 17, 2018 12:28

MrMage added 2 commits March 17, 2018 14:22

Add sample test suites to demonstrate how one would test client/serve…

e7645b4

…r code. These tests don't really test the logic of the SwiftGRPC library, but are meant as an example of how one would go about testing their own client/server code that relies on SwiftGRPC.

This was referenced Mar 17, 2018

Don't return "ok" when an operation group fails. #185

Closed

What's the best practice for testing SwiftGRPC clients/servers? #120

Closed

How to send errors back to the client? #123

Closed

MrMage added 3 commits March 17, 2018 15:22

Add a method to ServerSessionClientStreaming to return an error sta…

6af3914

…tus without a result.

Add a blocking variant of the send method to ServerStreaming.

8d07183

Add tests to verify the behavior of canceling client and server calls.

5d32204

Also changes a few method signatures where passing a `CallResult` does not make sense.

MrMage force-pushed the more-improvements branch from 7516d8d to 5d32204 Compare March 17, 2018 15:11

timburks reviewed Mar 17, 2018

View reviewed changes

timburks merged commit 951a20a into grpc:master Mar 17, 2018

This was referenced Mar 20, 2018

A bit of cleanup #191

Merged

Unary callback never executed #92

Closed

Delayed error response when network connection is not available #67

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

This week's improvements #184

This week's improvements #184

Uh oh!

MrMage commented Mar 12, 2018 •

edited

Loading

Uh oh!

SebastianThiebaud commented Mar 14, 2018

Uh oh!

MrMage commented Mar 14, 2018

Uh oh!

MrMage commented Mar 16, 2018

Uh oh!

MrMage commented Mar 17, 2018

Uh oh!

timburks Mar 17, 2018

Uh oh!

timburks Mar 17, 2018

Uh oh!

timburks left a comment

Uh oh!

timburks commented Mar 17, 2018

Uh oh!

Uh oh!

This week's improvements #184

This week's improvements #184

Uh oh!

Conversation

MrMage commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SebastianThiebaud commented Mar 14, 2018

Uh oh!

MrMage commented Mar 14, 2018

Uh oh!

MrMage commented Mar 16, 2018

Uh oh!

MrMage commented Mar 17, 2018

Uh oh!

timburks Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

timburks Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

timburks left a comment

Choose a reason for hiding this comment

Uh oh!

timburks commented Mar 17, 2018

Uh oh!

Uh oh!

MrMage commented Mar 12, 2018 •

edited

Loading