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

Revert "StatusException/StatusRuntimeException hide stack trace in a simpler way" #11066

Closed
wants to merge 1 commit into from

Conversation

sergiitk
Copy link
Member

@sergiitk sergiitk commented Apr 2, 2024

Internal linters caught an error in calling super in the constructor: Call requires API level 24 (current min is 19): new java.lang.Exception.

Reverts #11064.
Internal ref cl/621317657.

@sergiitk
Copy link
Member Author

sergiitk commented Apr 2, 2024

Not needed.

EDIT - updated in the next comment.

@sergiitk sergiitk closed this Apr 2, 2024
@sergiitk sergiitk deleted the revert-11064-this-escapes branch April 2, 2024 22:10
@sergiitk sergiitk restored the revert-11064-this-escapes branch April 3, 2024 21:17
@sergiitk sergiitk reopened this Apr 3, 2024
@sergiitk
Copy link
Member Author

sergiitk commented Apr 3, 2024

@panchenko - saw your comment in an email, but not here for some reason.

I am curious: is there a technical problem with that change?

I think we'd have to roll it back for the moment. Internal linters caught an error in calling super in the constructor: Call requires API level 24 (current min is 19): new java.lang.Exception.
Animal Sniffer configured in this repo didn't catch it for some reason.

@sergiitk sergiitk marked this pull request as ready for review April 3, 2024 21:25
@sergiitk sergiitk requested a review from ejona86 April 3, 2024 21:25
@ejona86
Copy link
Member

ejona86 commented Apr 3, 2024

The public docs make it seem like these exist since API 1.
https://developer.android.com/reference/java/lang/Exception#Exception(java.lang.String,%20java.lang.Throwable,%20boolean,%20boolean)

https://developer.android.com/reference/java/lang/RuntimeException#RuntimeException(java.lang.String,%20java.lang.Throwable,%20boolean,%20boolean)

Can that be right? These were added in Java 7, and Android started with Java 6; Java 7 didn't even exist when Android API level 1 was released. But our CI is also not crashing on these. Multiple of the tests should construct an exception:

I see a run that passed with API level 22.

@larry-safran
Copy link
Contributor

Since the thing this was going to revert has been in for 7 months, it seems that reverting is no longer needed. Can we close this PR?

@sergiitk sergiitk closed this Nov 11, 2024
@sergiitk sergiitk deleted the revert-11064-this-escapes branch November 11, 2024 23:08
ejona86 added a commit to ejona86/grpc-java that referenced this pull request Dec 18, 2024
In 61f19d7 I swapped the signatures to use the version catalog. But I
failed to preserve the `@signature` extension and it all seemed to
work... But in fact all the animalsniffer tasks were completing as
SKIPPED as they lacked signatures. The build.gradle changes in this
commit are to fix that while still using version catalog.

But while it was broken violations crept in. Most violations weren't
too important and we're not surprised went unnoticed. For example, Netty
with TLS has long required the Java 8 API
`setEndpointIdentificationAlgorithm()`, so using `Optional` in the same
code path didn't harm anything in particular. I still swapped it to
Guava's `Optional` to avoid overuse of `@IgnoreJRERequirement`.

One important violation has not been fixed and instead I've disabled the
android signature in api/build.gradle for the moment.  The violation is
in StatusException using the `fillInStackTrace` overload of Exception.
This problem [had been noticed][PR11066], but we couldn't figure out
what was going on. AnimalSniffer is now noticing this and agreeing with
the internal linter. There is still a question of why our interop tests
failed to notice this, but given they are no longer running on pre-API
level 24, that may forever be a mystery.

[PR11066]: grpc#11066
ejona86 added a commit that referenced this pull request Dec 19, 2024
In 61f19d7 I swapped the signatures to use the version catalog. But I
failed to preserve the `@signature` extension and it all seemed to
work... But in fact all the animalsniffer tasks were completing as
SKIPPED as they lacked signatures. The build.gradle changes in this
commit are to fix that while still using version catalog.

But while it was broken violations crept in. Most violations weren't
too important and we're not surprised went unnoticed. For example, Netty
with TLS has long required the Java 8 API
`setEndpointIdentificationAlgorithm()`, so using `Optional` in the same
code path didn't harm anything in particular. I still swapped it to
Guava's `Optional` to avoid overuse of `@IgnoreJRERequirement`.

One important violation has not been fixed and instead I've disabled the
android signature in api/build.gradle for the moment.  The violation is
in StatusException using the `fillInStackTrace` overload of Exception.
This problem [had been noticed][PR11066], but we couldn't figure out
what was going on. AnimalSniffer is now noticing this and agreeing with
the internal linter. There is still a question of why our interop tests
failed to notice this, but given they are no longer running on pre-API
level 24, that may forever be a mystery.

[PR11066]: #11066
@ejona86
Copy link
Member

ejona86 commented Dec 19, 2024

The riddle of the missing API has been semi-resolved: #11762 . I'm looking at pushing forward with #11072

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants