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.
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
C#: avoid shutdown crash on iOS #16308
C#: avoid shutdown crash on iOS #16308
Conversation
|
Successful package build: https://sponge.corp.google.com/invocation?id=39867719-113b-4f6e-8d5b-2118aab626ac&searchFor= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the syntax that well, but I guess the compiler does that check for me.
If checking that the type is available or not is enough, LGTM
isUnity = Type.GetType(UnityEngineApplicationClassName) != null; | ||
|
||
// Unity | ||
var unityApplicationClass = Type.GetType(UnityEngineApplicationClassName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this enough of a check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, there's no official way so we just chose an approach that seemed most reasonable (also the general approach stays the same in this PR, so not really an issue we need to solve right now).
isUnityIOS = false; | ||
} | ||
|
||
// Xamarin | ||
isXamarinIOS = Type.GetType(XamarinIOSObjectClassName) != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we won't have these types for anything else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems very unlikely someone would use that class name and assembly name. Also, see comment above.
// Normally grpc_init and grpc_shutdown calls should come in pairs (C core does reference counting), | ||
// but in case we avoid grpc_shutdown calls altogether, calling grpc_init has no effect | ||
// besides incrementing an internal C core counter that could theoretically overflow. | ||
// NOTE: synchronization not necessary here as we are only trying to avoid calling grpc_init |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the idea here that a race won't happen because it will never overflow quickly enough? In that case, even so, I think it would make it easier to read if alreadyInvokedNativeInit
was under a lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that it's not guaranteed that if we set alreadyInvokedNativeInit, other thread will immediately see that, but they will see it "very soon" and we don't mind invoking grpc_init once or twice in the meantime. In the worst case, each thread will run grpc_init once and that's still far from risking the owerflow.
The idea is the same as Joshua Bloch's "Racy-Single-Check" idiom -
http://javaagile.blogspot.com/2013/05/the-racy-single-check-idiom.html
which is used e.g. in Java String to initialize the hashCode field.
But you're right that this technique is a bit of an overkill for this situation. I'll double check that GrpcNativeInit only happens when a GrpcEnvironment gets initialized and probably use a lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd tend to err on the side of caution around this. And I really have no idea if this is an issue in C#, but see benign data races blog post
NativeMethods.Get().grpcsharp_init(); | ||
nativeInitCounter.Increment(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apolcyn because the GrpcNativeInit() is invoked for every channel creation / shutdown and server creation / shutdown, I wanted to avoid a global lock, so I used an AtomicCounter, to determine if grpcsharp_init needs to be invoked.
PTAL.
@@ -360,12 +361,25 @@ internal static string GetCoreVersionString() | |||
|
|||
internal static void GrpcNativeInit() | |||
{ | |||
if (!IsNativeShutdownAllowed && nativeInitCounter.Count > 0) | |||
{ | |||
// Normally grpc_init and grpc_shutdown calls should come in pairs (C core does reference counting), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Just another idea though: if going with this lock-free approach, I wonder if some kind of a "lock free once" algorithm would be better here. Also, the mutex vs. lock free counter thing seems like an interesting comparison to run/validate in the C# microbenchmarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you in general, but I didn't have a "lock free once" algorithm in mind (besides utilizing Lazy, which probably does doubly checked locking) and in the end using the AtomicCounter seemed like the simplest and most foolproof approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I can't really point to one.
Actually, can we not do something like:
{
if (nativeInitCounter.Increment() == 0) {
... call grpc_init()
}
return;
}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also thought of that but I think it's incorrect:
- thread 1 calls Increment, gets 0, then yields
- thread 2 calls Increment, gets 1 and thus skips grpc_init
- thread 2 does something assuming grpc_init has already been invoked, but that' not the case
- thread 1 finally invokes grpc_init, but it's too late
In another words, we must not increment before grpc_init has returned.
|
By skipping grpc_shutdown invocation. This is basically a hack, but it only affects iOS which is still supported by gRPC C# experimentally and this PR should increase the usability.
Tentative fix for #16294.