Two important fixes (and possibly more improvements) #188

MrMage · 2018-03-19T15:25:56Z

Fixed two critical memory leaks in the C shim, as well as another flake in GRPCTests.

…would all GRPC call objects to never get released, which in turn caused their completion queues and associated file descriptors to never get released. This became apparent after ~3-5k requests on macOS.

…tion).

MrMage · 2018-03-19T15:39:42Z

Trying to re-run CI...

…ue to the queue itself. This is needed because it appears that otherwise, the underlying completion queue gets deallocated during its spinloop, which it doesn't like.

…econds?

MrMage · 2018-03-19T17:33:04Z

Please don't merge this yet. I might have introduced new errors.

MrMage · 2018-03-19T17:50:33Z

Actually, I think it's fine ;-)

…#186.)

cvanderschuere

These test changes reverts the critical check of statusCode to mask an existing bug.

cvanderschuere · 2018-03-19T22:54:36Z

Tests/SwiftGRPCTests/GRPCTests.swift

  }

+  // We need to return status OK in both cases, as it seems like the server might never send out the last few messages


This seems like a huge problem. We need to assert that the server sends an appropriate status code and if it's not currently then tests should be failing

cvanderschuere · 2018-03-19T22:54:41Z

Tests/SwiftGRPCTests/GRPCTests.swift

@@ -157,23 +166,32 @@ func callUnary(channel: Channel) throws {
    try call.start(.unary, metadata: metadata, message: message) {
      response in
      // verify the basic response from the server
-      XCTAssertEqual(response.statusCode, (i % 2  == 0) ? evenStatusCode : oddStatusCode)


This means we never check for non .ok status codes which was the original bug filed in #144.

cvanderschuere · 2018-03-19T22:59:21Z

@timburks @MrMage Why was this merged? It invalidates work done for #144 in #147 & #183

MrMage · 2018-03-19T23:15:28Z

The tests you provided regularly had "unexpected response" messages (which I converted into XCTFail calls) and even occasional crashes (due to forcibly unwrapped optionals in the tests). It appears that if code returns a non-OK status, one can not rely on the gRPC library to still send all sent messages to the client. It took me several hours to figure this out. If you want tests that verify that a non-OK status code is sent, make sure to not send any messages back, as those might get dropped.

Also notice that we still have tests for this — it is now covered (and I'd say in a much cleaner way) by ServerThrowingTests.

MrMage · 2018-03-19T23:30:18Z

P.S.: I agree that we might want to raise the issue of the server not sending back all remaining messages when it is given a non-OK status to return with the gRPC-Core team. However, do keep in mind that this is now tested (with much less wrapping code that can cause unrelated flakes) in ServerThrowingTests, and I specifically left the part in that tests for custom status messages, given that status codes and messages are always sent together.

cvanderschuere · 2018-03-20T20:51:28Z

It appears that if code returns a non-OK status, one can not rely on the gRPC library to still send all sent messages to the client. It took me several hours to figure this out. If you want tests that verify that a non-OK status code is sent, make sure to not send any messages back, as those might get dropped.

This doesn't make any sense. Servers regularly return non-OK statuses and continue to operate. I think this is a problem specific to the swift implementation.

My concern is the haste in which these changes were made and the lack of debate over them. It doesn't inspire much faith in the project and quality users can expect from it.

timburks · 2018-03-20T21:30:46Z

@cvanderschuere Thanks for submitting tests and raising your concerns. Please consider everything since our last tagged version tentative.

MrMage · 2018-03-28T10:12:32Z

FYI, I have filed grpc/grpc#14842 with the gRPC project to inquire about the issue with non-OK status codes.

MrMage · 2018-04-03T10:12:30Z

Received an update on grpc/grpc#14842 (comment), with a link to https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/grpc-io/OwZZq27awUQ/uNRL0_7ICQAJ:

It is guaranteed if the status code of the RPC is OK. If the RPC terminates with any non-OK status, it is not guaranteed that the client will see all messages sent by the server.

The currently-supported behavior is that service-side abortion of an RPC (terminating an RPC with any status code other than OK) has "drop everything on the floor" semantics. The abortion renders the response message semantically meaningless, so the server-side gRPC runtime will not bother sending it if it hasn't already done so, and upon learning of the RPC abortion, the invocation-side gRPC runtime will not bother delivering the response (if it even received it) to the local application.

So I think the server will need to keep returning .ok, or the tests need to be rewritten. We still have ServerThrowingTests to check that non-OK status codes are sent, though.

MrMage added 2 commits March 19, 2018 16:23

Fix two huge memory leaks in cgrpc_call and cgrpc_handler. These …

78a1291

…would all GRPC call objects to never get released, which in turn caused their completion queues and associated file descriptors to never get released. This became apparent after ~3-5k requests on macOS.

Fix GRPCTests, hopefully once and for all (see 415307e for an explana…

d8ac7d4

…tion).

MrMage closed this Mar 19, 2018

MrMage reopened this Mar 19, 2018

MrMage added 2 commits March 19, 2018 17:42

Shift the responsibility for draining and destroying a completion que…

dc451e4

…ue to the queue itself. This is needed because it appears that otherwise, the underlying completion queue gets deallocated during its spinloop, which it doesn't like.

Try increasing the default timeout for server-cancelling tests to 5 s…

9b9ba22

…econds?

Add a simple API to provide a channel's connectivity state. (See grpc…

e68cec0

…#186.)

MrMage mentioned this pull request Mar 19, 2018

Thoughts on adding support for connectivity state #186

Closed

timburks merged commit dd4d1cb into grpc:master Mar 19, 2018

MrMage deleted the even-more-improvements branch March 19, 2018 18:46

cvanderschuere reviewed Mar 19, 2018

View reviewed changes

MrMage mentioned this pull request Mar 20, 2018

A bit of cleanup #191

Merged

MrMage mentioned this pull request Mar 28, 2018

Dropped response messages when sending non-OK status code grpc/grpc#14842

Closed

MrMage mentioned this pull request Apr 3, 2018

Further improvements #189

Closed

36 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Two important fixes (and possibly more improvements) #188

Two important fixes (and possibly more improvements) #188

MrMage commented Mar 19, 2018

MrMage commented Mar 19, 2018

MrMage commented Mar 19, 2018

MrMage commented Mar 19, 2018

cvanderschuere left a comment

cvanderschuere Mar 19, 2018

cvanderschuere Mar 19, 2018

cvanderschuere commented Mar 19, 2018

MrMage commented Mar 19, 2018

MrMage commented Mar 19, 2018

cvanderschuere commented Mar 20, 2018

timburks commented Mar 20, 2018

MrMage commented Mar 28, 2018

MrMage commented Apr 3, 2018 •

edited

Loading

		}

		// We need to return status OK in both cases, as it seems like the server might never send out the last few messages

Two important fixes (and possibly more improvements) #188

Two important fixes (and possibly more improvements) #188

Conversation

MrMage commented Mar 19, 2018

MrMage commented Mar 19, 2018

MrMage commented Mar 19, 2018

MrMage commented Mar 19, 2018

cvanderschuere left a comment

Choose a reason for hiding this comment

cvanderschuere Mar 19, 2018

Choose a reason for hiding this comment

cvanderschuere Mar 19, 2018

Choose a reason for hiding this comment

cvanderschuere commented Mar 19, 2018

MrMage commented Mar 19, 2018

MrMage commented Mar 19, 2018

cvanderschuere commented Mar 20, 2018

timburks commented Mar 20, 2018

MrMage commented Mar 28, 2018

MrMage commented Apr 3, 2018 • edited Loading

MrMage commented Apr 3, 2018 •

edited

Loading