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

unhandled exception ends up with error message indicating proto3_optional isn't supported #1156

Open
noahdietz opened this issue Jun 30, 2022 · 2 comments
Assignees
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@noahdietz
Copy link
Contributor

If there is an unhandled exception thrown by gapic-generator-typescript generating an API leveraging proto3_optional, the logs (at least in bazel) indicate that there was an error because the generator did not respond to protoc with the fact that it supports proto3_optional, see the following logs: https://screenshot.googleplex.com/4AQCdHQoJDU8uGS.

This might actually make sense because the plugin itself never "responds" to protoc (because it crashed), and protoc then logs that the plugin failed because it doesn't support proto3_optional.

This might actually need to be fixed in protoc, but perhaps the gapic-generator-typescript plugin runner should capture all exceptions, respond to protoc with an error response (and a proper flag indicating it supports proto3_optional) using the "unhandled" exception. I'm not sure, but it did add indirection to debugging this http://yaqs/5695674741042446336.

@noahdietz noahdietz added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p3 Desirable enhancement or fix. May not be included in next release. labels Jun 30, 2022
@alexander-fenster
Copy link
Contributor

alexander-fenster commented Jun 30, 2022

Yeah, we don't have a catch-all try .. catch:

https://github.com/googleapis/gapic-generator-typescript/blob/68f5160bf5781848a38957a550c0f4eeb83e2fd1/typescript/src/protoc-plugin.ts#L21..L33

In a fire-and-forget code such as a generator we normally prefer not to catch any exceptions but throw a lot, so that we just crash immediately in any suspicious case (thus ignoring the protoc expectations on the error reporting from a plugin). It actually gives more, not less, information because this way we see a stack that immediately points to a place where the error happened, compared to the protoc error interface that only has optional string error = 1;.

But if this makes Bazel happier I can totally wrap the plugin invocation in try .. catch :)

@noahdietz
Copy link
Contributor Author

Yeah the protoc error interface is not that robust. Could we just stuff the entire exception trace in the error string returned to protoc? Or just log the entire trace from the generator prior to responding with an error? Or is there stuff that typescript/nodejs itself is doing that we'd lose by doing that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

3 participants