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.

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

Bunch 'O Shutdown Fixes #2492

Merged
merged 16 commits into from
Dec 21, 2018
Merged

Bunch 'O Shutdown Fixes #2492

merged 16 commits into from
Dec 21, 2018

Conversation

pmuetschard
Copy link
Member

The gapis and gapir shutdown handling was, well, quite broken. This fixes things. The most noticeable thing this fixes is Android gapir: it no longer uses a full core while idling and the app will exit when the client exits.

gapis fixes:

  • Most of the "at exit" clean stuff wasn't running, because we ignored the SIGTERM signal, cancelled contexts would make things be ignored and because of deadlocks.
  • Close the log handler at the very end, so stuff still gets logged while cleaning up
  • When wanting to shutdown the gRPC server, cancel any log streaming RPCs, since they would cause the server to stay running indefinitely.
  • Send the Shutdown RPC to gapir.

gapir fixes:

  • Use a 1 second timeout in ALooper_pollAll call, because 0 causes it to return immediately and the tight loop causes a full core to spin.
  • Exit the app when gapir exits.
  • Actually monitor whether the gRPC server is still running in the main loop.
  • Correctly handle the Shutdown RPC request.

@pmuetschard pmuetschard force-pushed the shutdown branch 2 times, most recently from 36b2997 to c1a39d8 Compare December 20, 2018 01:06
@@ -143,6 +143,46 @@ std::unique_ptr<Server> Setup(const char* uri, const char* authToken,

#if TARGET_OS == GAPID_OS_ANDROID

namespace {

#define JNI_CALL_O(obj, name, sig, ...) \
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to see this as a template, rather than a macro.

Pardon my bad pseudo-code and making up names of JNI api things:

template<typename... Args>
jobject jni_call_o(Env* env, clazz obj, const char* name, const char* sig, Args&&... args) {
   return env->CallObjectMethod(obj, env->GetMethodIf()......, std::forward<Args>(args)...);
}

Copy link
Contributor

@AWoloszyn AWoloszyn left a comment

Choose a reason for hiding this comment

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

Approved, but I would really prefer to see the Macro turned into a template, since macros make me sad.

Don't drop log messages on exit on the floor.
`session.close` invokes the callback from `getOrCreateSession`, which
aquires the mutex. Thus `session.close` has to be called without holding
the mutex.
Usually gapir is shutdown, when gapis is shutting down, which means the
context is cancelled, and gRPC would ignore it.
`rpcServer->Shutdown` blocks until all RPCs have finished. Calling it from
within an RPC, well, deadlocks.

Also correctly sets and fixes the usage of the `mShuttingDown` latch.
@pmuetschard pmuetschard merged commit 646a144 into google:master Dec 21, 2018
@pmuetschard pmuetschard deleted the shutdown branch December 21, 2018 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants