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

x/mobile/bind/java: translate panic to a java error #11382

Closed
jamesbikes opened this issue Jun 24, 2015 · 11 comments
Closed

x/mobile/bind/java: translate panic to a java error #11382

jamesbikes opened this issue Jun 24, 2015 · 11 comments

Comments

@jamesbikes
Copy link

A panic() from Go code eventually results in a syscall to exit(2) which terminates the Android app. This "clean" exit prevents Crashlytics (https://get.fabric.io/crashlytics) from capturing a Go panic as a crash.

Crashlytics support told me:

We do install a signal handler to capture the crashes. You could write a custom panic() function in Go that uses the OS package to raise a signal, but we are hestitant to treat an exit() as a crash in the native code as it would be the program telling the OS that everything is fine.

Could we recover these panics and throw a Java Error? Or perhaps syscall abort (which should be caught by a SIGABRT handler) instead of exit?

@hyangah
Copy link
Contributor

hyangah commented Jun 24, 2015

where is the panic from?
If it occurs during executing the go function called from java, it's an option to make your go function recover, convert it to an error, and return an error which will be translated into a java exception.

@jamesbikes
Copy link
Author

The scenario I was testing was a Go function called from Java. Would be nice not to have to wrap all my external functions with panic/recover and have the runtime just support this, but maybe I'm asking too much.

To be a bit clearer, I'm not expecting any of my Go code to panic. But if I'm accessing an array out of bounds and the app is crashing for half my users, I want to have reporting that tells me that.

@hyangah hyangah changed the title x/mobile/cmd/gomobile: panic looks like clean exit x/mobile/bind/java: translate panic to a java exception Jun 25, 2015
@hyangah
Copy link
Contributor

hyangah commented Jun 25, 2015

one option David Crawshaw suggested is to make gobind generate a code that recover and translate the panic to java exception.
cc @crawshaw

@crawshaw
Copy link
Member

A Go panic has a lot of technical similarities to Java exceptions, but several conventional differences. Java exceptions are part of normal programming, Go panics are not.

We may be able to match the conventions by throwing a java.lang.Error. While throwable, it's not a subclass of java.lang.Exception, so the usual try { } catch (Exception e) {} boilerplate won't get catch it.

(It is more than a little confusing that Java Errors are like Go panics, and Java Exceptions are like Go errors.)

The only concern left is any potential performance impact from the defer/recover block, but I believe the language barrier will always incur a more significant overhead.

@hyangah hyangah changed the title x/mobile/bind/java: translate panic to a java exception x/mobile/bind/java: translate panic to a java error Jun 26, 2015
@jamesbikes
Copy link
Author

I took a stab at implementing this: https://go-review.googlesource.com/#/c/11920/

Feedback welcome. Haven't written C in years, and JNI code in... ever. :)

@gopherbot
Copy link

CL https://golang.org/cl/11920 mentions this issue.

@eliasnaur
Copy link
Contributor

What was wrong with the suggestion of replacing exit(2) with abort(3)? Not every Go call originates from java, and it seems more appropriate for a library (libgojni.so) to abort rather than exit the foreign main program.

@hyangah
Copy link
Contributor

hyangah commented Jul 7, 2015

after chatting with @alandonovan the other day, I also realized that turning the panic into a java error addresses only part of the problem. There could be background goroutine causing crash, some library underneath deciding to crash, gc panics, or deadlock(not sure if it applies to library). In order to capture these cases, I also wish we could use abort instead of exit. @crawshaw @ianlancetaylor

@jamesbikes
Copy link
Author

That makes sense to me. Any benefit in translating to Error where we can (i.e. in Go code directly invoked from Java) and aborting when we can't? Maybe cleaner stack traces on the Errors?

@hyangah
Copy link
Contributor

hyangah commented Jul 7, 2015

@rsc pointed out runtime has already a way to change the program crash behavior by setting GOTRACEBACK env var and we may be able to utilize the similar hook.

http://golang.org/pkg/runtime/

"If GOTRACEBACK=crash, the per-goroutine stack traces include run-time functions, and if possible the program crashes in an operating-specific manner instead of exiting. For example, on Unix systems, the program raises SIGABRT to trigger a core dump."

On linux, the SIGABRT is handled and creates a core dump. On mobile, change the full stack trace to the Errors?

@eliasnaur
Copy link
Contributor

CL 12032 is my proposal for fixing this issue. GOTRACEBACK is read too early in the runtime initialization to allow for gomobile to fiddle with GOTRACEBACK and in any case I believe the aborting behaviour should cover all cases where C owns the process. In any case, GOTRACEBACK=crash behaviour includes GOTRACEBACK=2 behaviour (print runtime functions in tracebacks) which is not what we want here.

@golang golang locked and limited conversation to collaborators Jul 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants