Fix crash during Channel with subscription is destroying #328
Conversation
|
Thank you! Would it be possible to add a test case for this as well that would fail without your fixes? (A proper review will follow, I'm on the go right now.) |
|
I've added a test case in ChannelCrashTests.swift. You need just comment my changes in Channel.swift to reproduce the crash. Thank you for quick respond! |
slavabulgakov
force-pushed
the
Fix-crash-during-Channel-destroying
branch
from
b5598b8 to
557aa4e
Compare
There was a problem hiding this comment.
Thank you for these fixes! My suspicion is that this crash could still happen in some rare conditions due to a race condition where the channel is shut down in between the hasBeenShutdown check and the cgrpc_channel_check_connectivity_state call. I.e. if the channel is closed from a different thread while the loop is somewhere between line 166 and 172, this could still crash.
I don't have a good solution for this off the top of my head, but at least this should also happen much more rarely with your fixes.
| import SwiftGRPC | ||
|
|
||
| class ChannelCrashProvider: ChannelCrash_ChannelCrashProvider { | ||
|
|
There was a problem hiding this comment.
Nit: please remove this newline.
| import Foundation | ||
| import SwiftGRPC | ||
|
|
||
| class ChannelCrashProvider: ChannelCrash_ChannelCrashProvider { |
There was a problem hiding this comment.
Could you pull this entire class into ChannelCrashTests.swift?
| @@ -0,0 +1,108 @@ | |||
| // | |||
There was a problem hiding this comment.
Could you use the existing generated code for EchoService instead, and move ChannelCrashTests.swift into the SwiftGRPCTests folder?
Sources/SwiftGRPC/Core/Channel.swift
Outdated
| @@ -151,11 +152,20 @@ private extension Channel { | |||
|
|
|||
| spinloopThreadQueue.async { | |||
| while true { | |||
|
|
|||
There was a problem hiding this comment.
nit: please remove this newline
| } | ||
|
|
||
| class ChannelCrashTests: XCTestCase { | ||
|
|
There was a problem hiding this comment.
Sorry for noticing this only now, but could you please make this a subclass of https://github.com/grpc/grpc-swift/blob/master/Tests/SwiftGRPCTests/BasicEchoTestCase.swift? That should spare you from all the client/server setup work. Also, please remove this extra newline.
| } | ||
|
|
||
| private func receive(call: Echo_EchoExpandCall) { | ||
| try? call.receive { result in |
There was a problem hiding this comment.
Please use try! and move this into the test method itself.
There was a problem hiding this comment.
Do you suggest to move call "try! call.receive {...}" inside "testChannelCrash" method?
If yes then I cannot recursively call method "private func receive(call: Echo_EchoExpandCall)" as I do it now
There was a problem hiding this comment.
Could you instead use the blocking, synchronous variant of receive? Or leave out the receive part completely, given that it should not be relevant to the crash, anyway?
There was a problem hiding this comment.
Agreed, it is not relevant to the crash. I will remove it.
| var server: ServiceServer! | ||
| var client: Echo_EchoServiceClient? | ||
|
|
||
| func testChannelCrash() { |
There was a problem hiding this comment.
nit: Rename to testDanglingConnectivityObserversDontCrash?
| func expand(request: Echo_EchoRequest, session: Echo_EchoExpandSession) throws -> ServerStatus? { | ||
| let parts = request.text.components(separatedBy: " ") | ||
| for (i, part) in parts.enumerated() { | ||
| usleep(500000) |
| receive(call: call) | ||
|
|
||
| DispatchQueue.main.asyncAfter(deadline: .now() + .seconds(1)) { | ||
| self.client = nil |
There was a problem hiding this comment.
Please add a comment that this is to ensure that the client gets deallocated. Also, can all the timeouts be shortened quite a bit, to avoid increasing the test runtime so much?
| @@ -0,0 +1,83 @@ | |||
| // | |||
There was a problem hiding this comment.
Please remove this comment block.
There was a problem hiding this comment.
Is it necessary to replace it with your block "Copyright 2018, gRPC Authors All ..." which used in other files?
There was a problem hiding this comment.
I think that would be best, thanks for spotting!
| @@ -0,0 +1,34 @@ | |||
| // Copyright (c) 2015, Google Inc. | |||
Sources/SwiftGRPC/Core/Channel.swift
Outdated
| @@ -178,6 +187,7 @@ private extension Channel { | |||
| } | |||
|
|
|||
| func shutdown() { | |||
| hasBeenShutdown = true | |||
There was a problem hiding this comment.
As mentioned before, please synchronize accesses to this variable.
| @testable import SwiftGRPC | ||
|
|
||
| class ChannelCrashTests: BasicEchoTestCase { | ||
|
|
There was a problem hiding this comment.
nit: please remove this newline.
Sources/SwiftGRPC/Core/Channel.swift
Outdated
| @@ -151,11 +153,19 @@ private extension Channel { | |||
|
|
|||
| spinloopThreadQueue.async { | |||
| while true { | |||
| guard !self.hasBeenShutdown else { | |||
There was a problem hiding this comment.
Please also wrap these two checks in a channelMutex.synchronize call, e.g.
guard (channelMutex.synchronize { !self.hasBeenShutdown } ...
Sources/SwiftGRPC/Core/Channel.swift
Outdated
| @@ -132,6 +132,8 @@ private extension Channel { | |||
| private let underlyingCompletionQueue: UnsafeMutableRawPointer | |||
| private let callback: (ConnectivityState) -> Void | |||
| private var lastState: ConnectivityState | |||
| private var hasBeenShutdown = false | |||
| private let channelMutex: Mutex = Mutex() | |||
Fix for #327
The goal was to fix crash during Channel (which has subscription to connectivity state changes) is destroying