Skip to content

Conversation

mrts
Copy link
Member

@mrts mrts commented May 9, 2023

WE2-790

@mrts mrts mentioned this pull request May 9, 2023
@mrts mrts added this to the v3 milestone May 9, 2023
@mrts mrts changed the title Switch to Java 11 to use the built-in HttpClient (version 3) Switch to Java 11 to use the built-in HttpClient instead of OkHttpClient (version 3) May 9, 2023
@mrts mrts changed the title Switch to Java 11 to use the built-in HttpClient instead of OkHttpClient (version 3) Switch to Java 11 to use the built-in HttpClient instead of OkHttpClient May 9, 2023
@mrts mrts force-pushed the switch-to-java-11-and-builtin-httpclient branch 3 times, most recently from 18bf2ba to 07aaa35 Compare May 16, 2023 15:03
Copy link

@angryziber angryziber left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Rethrowing InterruptedException is suspicious, but probably ok to preserve the interface signature.

@angryziber
Copy link

Maybe class name could be improved to something like OcspClientImpl, because it's the only one and uses only built-in JDK dependencies.

@mrts
Copy link
Member Author

mrts commented May 19, 2023

Thanks, much appreciated!

Yes, I intended to preserve the signature. Also, exposing InterruptedException in the interface signature would require that the developers know that they have to call Thread.currentThread().interrupt() before rethrowing it. I think this puts too large onus on them and may cause problems, so it is better to catch it, handle the thread interruption and rethrow as IOException ourselves. The assumption is that the interruption is a rare event and developers want to treat it like any other I/O failure in the network stack in the particular case of HTTP requests.

However, the risks include confusing the interrupt signal with an I/O issue. They are fundamentally different issues: an interrupt generally means the thread has been asked to stop what it's doing, while an IOException indicates some kind of error has occurred during I/O. So, formally it is misleading to conflate them.

We can change the API in a backwards-incompatible manner only in a major version, so it is a now or never type of decision regarding the signature of the most important method AuthTokenValidator.validate() and the stakes are high.

What do you think about this? Can you bring an example where handling InterruptedException inside the library would be a problem even though the the interrupted status of the thread is maintained with Thread.currentThread().interrupt()?

Regarding naming: thank you, I agree, OcspClientImpl is a better name considering that it is the only implementation and uses only built-in JDK dependencies. This has been fixed now. BuiltinHttpClientOcspClient is precise, but hard to read and justified only in case there are many implementations.

@mrts mrts force-pushed the switch-to-java-11-and-builtin-httpclient branch 7 times, most recently from 1daf0e4 to 0e86aa5 Compare August 4, 2023 18:22
@mrts mrts requested a review from metsma August 5, 2023 05:49
@mrts mrts linked an issue Aug 5, 2023 that may be closed by this pull request
Signed-off-by: Mart Somermaa <mrts@users.noreply.github.com>
@mrts mrts force-pushed the switch-to-java-11-and-builtin-httpclient branch from 0e86aa5 to 9293f3f Compare August 7, 2023 10:52
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

81.2% 81.2% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@mrts mrts merged commit 00e4ecd into main Aug 7, 2023
@mrts mrts deleted the switch-to-java-11-and-builtin-httpclient branch August 7, 2023 12:43
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.

Version 3 requires Java 11

3 participants