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

Rollback to official source when any errors occur in the image #6636

Open
wants to merge 4 commits into
base: v3_openjdk
Choose a base branch
from

Conversation

f401
Copy link

@f401 f401 commented Feb 17, 2025

No description provided.

@Mathias-Boulay
Copy link
Contributor

Hi there @f401, thank you for bringing your concern through a contribution.

However, it would be preferable to know what kind of Exception we want to catch from the mirror. If you want to catch more exception types to handle them differently, be less broad than this in your catch statement.

@Mathias-Boulay Mathias-Boulay self-requested a review February 22, 2025 21:37
Copy link
Contributor

@Mathias-Boulay Mathias-Boulay left a comment

Choose a reason for hiding this comment

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

The handling regarding some exceptions leaves a bit to be desired.
For example, if one was to have the unknown host exception on a single file from a mirror, it is pretty much expected for said exception to resurface for every single file.

Suggestion: Make the DownloadMirror class non-static and make it aware of the failures when trying to download from a mirror.

@@ -30,21 +29,16 @@ public static void download(URL url, OutputStream os) throws IOException {
conn.setDoInput(true);
conn.connect();
if (conn.getResponseCode() != HttpURLConnection.HTTP_OK) {
throw new IOException("Server returned HTTP " + conn.getResponseCode()
// We may be on the mirror, just give it a chance to rollback to official
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should not be there. download is a general purpose function.

Suggestion: remove the comment

Copy link
Author

Choose a reason for hiding this comment

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

Removed comment

@@ -30,21 +29,16 @@ public static void download(URL url, OutputStream os) throws IOException {
conn.setDoInput(true);
conn.connect();
if (conn.getResponseCode() != HttpURLConnection.HTTP_OK) {
throw new IOException("Server returned HTTP " + conn.getResponseCode()
// We may be on the mirror, just give it a chance to rollback to official
throw new SocketException("Server returned HTTP " + conn.getResponseCode()
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong Exception kind, as on a technical level, the communication was executed successfully.

Suggestion: Create an alternate Exception type like HttpException

Copy link
Author

Choose a reason for hiding this comment

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

Created HttpException

}
} catch (IOException e) { // Here Is Socket Exception
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: remove comment

Copy link
Author

Choose a reason for hiding this comment

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

removed comment

@f401 f401 requested a review from Mathias-Boulay February 24, 2025 08:59
@f401
Copy link
Author

f401 commented Feb 24, 2025

The handling regarding some exceptions leaves a bit to be desired. For example, if one was to have the unknown host exception on a single file from a mirror, it is pretty much expected for said exception to resurface for every single file.

Suggestion: Make the DownloadMirror class non-static and make it aware of the failures when trying to download from a mirror.

According to https://github.com/bangbang93/openbmclapi.
bmclapi distributes files through OpenBMCLAPI, and the inability to download a single file does not mean that the entire mirror is unavailable

@Mathias-Boulay
Copy link
Contributor

According to https://github.com/bangbang93/openbmclapi. bmclapi distributes files through OpenBMCLAPI, and the inability to download a single file does not mean that the entire mirror is unavailable

This is indeed right. However, my statement is right as well, but allow me to reformulate it for you:
Some exceptions like UnknownHostException can happen. Http error codes 500+ are valid reason to fallback on the official mirror for all download if encountered. Missing files on a mirror should return 404 (hopefully) or 403/401.

Copy link
Contributor

@Mathias-Boulay Mathias-Boulay left a comment

Choose a reason for hiding this comment

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

Somes changes have to be made, see my comment outside of the review as well.


public class HttpException extends IOException {
// Do not change. Android really hates when this value changes for some reason.
private static final long serialVersionUID = -7372301619612640655L;
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Can you elaborate on why this line is here ?
In my limited testing, I failed to see what it changed.

// Do not change. Android really hates when this value changes for some reason.
private static final long serialVersionUID = -7372301619612640655L;

public HttpException(String msg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: The Exception should take an http code accessible to the catch block of Exception
handlers.

@f401
Copy link
Author

f401 commented Feb 26, 2025

@Mathias-Boulay
Please allow me to explain again (please forgive my poor English level):
Firstly, the exceptions I added in the catch block were all obtained through actual testing. (Thank you for forgiving my poor programming language skills and your patience)
There are two situations where an UnknownHostException occurs: https://bmclapi2.bangbang93.com itself throwing an UnknownHostException or an UnknownHostException distributed to nodes by openbmclapi. In my understanding, what you are referring to is the former. The former is because the image did indeed fail, and the UnknownHostException I obtained during my testing was the latter. Similarly, it is uncertain whether HTTP Code 500+ is thrown by the former or the latter. Additionally, please find attached a test code from me

import java.io.FileOutputStream
import java.io.IOException
import java.net.HttpURLConnection
import java.net.URL

fun main() {
    val url = "https://bmclapi2.bangbang93.com/version/1.7.10/client"
    var hitCount = 0
    var code: Int = -1;
    do {
        val connection =
            URL(url).openConnection() as HttpURLConnection
        try {
            connection.connect()
            code = connection.responseCode
            ++hitCount
            val fos = FileOutputStream("/tmp/t.jar")
            connection.inputStream.copyTo(fos)
            fos.close()
        } catch (e: IOException) {
            e.printStackTrace()
        }

        connection.disconnect()
    } while (code == HttpURLConnection.HTTP_OK)
    println("Code $code, hit count $hitCount")
}

result:

java.io.FileNotFoundException: https://download.cloud.189.cn/notfound/notfound.jsp
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481)
	at java.base/sun.net.www.protocol.http.HttpURLConnection$10.run(HttpURLConnection.java:2093)
	at java.base/sun.net.www.protocol.http.HttpURLConnection$10.run(HttpURLConnection.java:2088)
	at java.base/java.security.AccessController.doPrivileged(AccessController.java:569)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getChainedException(HttpURLConnection.java:2087)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1645)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1625)
	at java.base/sun.net.www.protocol.https.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:224)
	at org.example.MainKt.main(Main.kt:20)
	at org.example.MainKt.main(Main.kt)
Caused by: java.io.FileNotFoundException: https://download.cloud.189.cn/notfound/notfound.jsp
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:2032)
	at java.base/sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1625)
	at java.base/java.net.HttpURLConnection.getResponseCode(HttpURLConnection.java:529)
	at java.base/sun.net.www.protocol.https.HttpsURLConnectionImpl.getResponseCode(HttpsURLConnectionImpl.java:308)
	at org.example.MainKt.main(Main.kt:17)
	... 1 more
Code 404, hit count 174

It can be seen that the probability of encountering a 404 error when downloading the same file is low. It also confirms that there may be issues with the child nodes. Here is also an unresolved issue with OpenBMCLAPI regarding the validation of HTTP Code availability for child nodes (written in Chinese) bangbang93/openbmclapi#8. My personal opinion: The above is due to implementation issues with the BMCLAPI source. It cannot be ruled out that other sources may be added in the future. Perhaps internal service errors in the download source can be resolved by rolling back the subsequent downloads to the official source (your suggestion) as a launcher preference option.

@f401 f401 requested a review from Mathias-Boulay February 28, 2025 11:44
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.

2 participants