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

Catches IOException in DownloadUtils::isJenkinsAvailable #437

Merged
merged 2 commits into from Jun 20, 2019

Conversation

Projects
None yet
4 participants
@praj-foss
Copy link
Member

commented Jun 19, 2019

Summary

Catches IOException while working with HTTP connection inside DownloadUtils::isJenkinsAvailable. Fixes #435.

@praj-foss praj-foss requested a review from skaldarnar Jun 19, 2019

@GooeyHub

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

Hooray Jenkins reported success with all tests good!

@praj-foss praj-foss added the GSoC 2019 label Jun 19, 2019

@skaldarnar
Copy link
Member

left a comment

The fix is what I was originally aiming for 👍

However, I had another look at the method and I'm wondering whether could simplify it (and honestly, maybe just stick with catching the general Exception as we have to do anyways):

public static boolean isJenkinsAvailable() {
        logger.trace("Checking Jenkins availability...");
        try {
            HttpURLConnection conn = (HttpURLConnection) new URL(JENKINS_URL).openConnection();
            try (AutoCloseable ac = conn::disconnect) {
                conn.setConnectTimeout(JENKINS_TIMEOUT);
                if (conn.getResponseCode() == HttpURLConnection.HTTP_OK) {
                    logger.trace("Jenkins is available at {}", JENKINS_URL);
                    return true;
                }
            } 
        } catch (Exception e) {
            logger.warn("Could not reach Jenkins at {} - {}", JENKINS_URL, e.getMessage());
        }
        return false;
    }

What do you think about that?

@@ -204,8 +204,11 @@ public static boolean isJenkinsAvailable() {
throw new ConnectException();

This comment has been minimized.

Copy link
@skaldarnar

skaldarnar Jun 20, 2019

Member

I don't get we throw an exception here - coudn't we just return false;?

This comment has been minimized.

Copy link
@praj-foss

praj-foss Jun 20, 2019

Author Member

Well, I did it just to trigger its external catch(IOException) block, where logging happens. All other obstacles will lead to the same catch block too. Is there any problem with such flow?

logger.warn("Could not connect to Jenkins at {}", JENKINS_URL);
} catch (Exception e) {

This comment has been minimized.

Copy link
@skaldarnar

skaldarnar Jun 20, 2019

Member

I had a closer look at what exceptions can occur for what reason:

  • MalformedURLException in case our static string is not a valid URL - should never happen, and is caught by Exception anyways
  • IOException can occur for multiple reasons (.openConnection(), .getResponseCode(), …)
  • Exception due to AutoClosable interface - we have to catch this

This comment has been minimized.

Copy link
@praj-foss

praj-foss Jun 20, 2019

Author Member

I agree with you. Note that MalformedURLException is a child of IOException, so the first block will handle it actually. The catch(Exception) block is present exclusively due to the AutoCloseable object, and it'll be triggered only when HttpURLConnection::disconnect goes wrong (which itself throws IOException).

@praj-foss praj-foss self-assigned this Jun 20, 2019

@skaldarnar

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

@GooeyHub ok to test

@skaldarnar skaldarnar merged commit 1841723 into develop Jun 20, 2019

1 check was pending

default Build triggered. sha1 is merged.
Details

@skaldarnar skaldarnar deleted the feature/fix-435 branch Jun 20, 2019

@GooeyHub

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Uh oh, something went wrong with the build. Need to check on that

1 similar comment
@GooeyHub

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Uh oh, something went wrong with the build. Need to check on that

@Cervator Cervator added this to the v4.0.0 - GSOC 2019 milestone Jun 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.