chore(JenkinsClient): use new HTTP client with better diagnostics #656

keturn · 2021-08-18T02:30:53Z

I was trying to review another PR and I noticed none of the preview releases were showing up. One of our Jenkins-as-repository / HTTPS / redirect sorts of issues. So I tried swapping out the JenkinsClient#openStream implementation for one that used the newer HttpClient from Java 11.

We do get more control of how redirects are handled and more visibility in to the HTTP response if needed for troubleshooting.

Looks to be successfully following the redirects too.

It is also awfully verbose for this simple use case.

then I spent like two hours going through mailing list archives looking for the part where someone explained why HttpRequest takes a URI, not a URL. Surely that must have been a FAQ while it was going through its development and incubator phase, right? But I haven't found an answer yet, or even anyone on the net-dev list raising the question.

We get more control of how redirects are handled and more visibility in to the HTTP response if needed for troubleshooting.

keturn · 2021-08-18T02:50:23Z

This is the failing test:

TerasologyLauncher/src/test/java/org/terasology/launcher/repositories/JenkinsClientTest.java

Lines 29 to 39 in 72dbf73

    
           @Test 
        
           @DisplayName("should handle IOException on request(url) gracefully") 
        
           void nullOnIoException() throws IOException, InterruptedException { 
        
               final Gson gson = new Gson(); 
        
               final JenkinsClient client = new JenkinsClient(gson); 
        
               URL urlThrowingException = mock(URL.class); 
        
               doThrow(IOException.class).when(urlThrowingException).openConnection(); 
        
               assertNull(client.request(urlThrowingException)); 
        
           }

which uses mockito to throw an exception when the given URL's openConnection method is called.

But HttpClient never calls url.openConnection.

I guess I should make that test override client#openStream instead.

keturn · 2021-08-18T15:29:34Z

I then discovered this results in JenkinsClient being able to follow redirects to get the properties file, but then DownloadUtils isn't able to download the corresponding release. 🤦

This tries to be a minimal change from the previous version, but the mixture of streaming APIs used is not pretty.

keturn

keturn · 2021-08-19T01:34:34Z

InterruptedException

I'm reluctant to catch InterruptedException because it feels like a sign that something is trying to shut everything down. How far should we let it bubble up?

Or maybe this is the wrong interpretation. Because it's not the current thread that's been interrupted—that was some reader thread that we were waiting on. Maybe that should be considered an implementation detail and it should be wrapped up in a DownloadException like most anything else.

HttpClient

We don't really want to be creating a new HttpClient instance for every request. Especially since we tend to make a lot of requests against the same host. It'd be nice to move that to some Service or other singleton. That would let us move those HttpClient configuration lines out of the request methods too.

It's not a huge deal, but we could probably shave off some time from the big stack of requests JenkinsClient does to get a displayVersion for every build.

DownloadTask & DownloadUtils.downloadToFile

In the name of incremental progress (and because this rabbit hole was already far enough from where I started out), I tried to leave most of the existing DownloadUtils stuff intact and mostly just swap out the part responsible for getting the InputStream started.

But it's all a jumble of concurrency and stream models. I was thinking downloadToFile should return a Task, instead of requiring ProgressListener to be passed in and not making any use of its return value.

then I find out we already have a DownloadTask extends Task class.
and there's this asynchronous progress reporting, but the code calling downloadToFile really expects it to block until completion.

It wouldn't be a huge project to rewire, but more than I wanted to bite off without checking for scope creep.

There's also the part where Task is JavaFX-specific, and that work would be for the purpose of supporting the JavaFX progress bar, and @skaldarnar has been mumbling about throwing out JavaFX entirely, so I don't know if it's a good time to invest in JavaFX-specific code.

instead of relying on the URL argument to be used in a specific way

keturn · 2021-08-19T02:22:55Z

I almost got to write that without mockito, but "static declarations in inner classes" are not supported until language level 16.

anyway, test is fixed now, so taking this out of Draft status.

skaldarnar

I like those changes 👍

Feel free to either merge straight away or address the review comments - nothing blocking here from my side.

skaldarnar · 2021-08-19T19:58:30Z

src/main/java/org/terasology/launcher/game/GameManager.java

@@ -79,7 +80,7 @@ public void install(GameRelease release, ProgressListener listener) throws IOExc
        }
    }

-    private void download(GameRelease release, Path targetLocation, ProgressListener listener) throws DownloadException, IOException {
+    private void download(GameRelease release, Path targetLocation, ProgressListener listener) throws DownloadException, IOException, InterruptedException {


I'm also not sure about the InterruptedException. I would have thought that this is similar to any other exception that could cause the download to fail...

However, I do think this should be pretty rare, and unless we get a bunch of complaints from users we should be good either way.

skaldarnar · 2021-08-19T20:05:11Z

src/main/java/org/terasology/launcher/repositories/JenkinsClient.java

-        URLConnection connection = url.openConnection();
-        connection.setConnectTimeout(10 * 1000);
-        return connection.getInputStream();
+        var client = HttpClient.newBuilder()


Should we add a //TODO: ... to reuse one client for multiple requests?

skaldarnar · 2021-08-19T20:05:23Z

src/main/java/org/terasology/launcher/repositories/JenkinsClient.java

-        connection.setConnectTimeout(10 * 1000);
-        return connection.getInputStream();
+        var client = HttpClient.newBuilder()
+                .connectTimeout(Duration.ofSeconds(10))


skaldarnar · 2021-08-19T20:11:03Z

src/main/java/org/terasology/launcher/repositories/JenkinsClient.java

+                .followRedirects(HttpClient.Redirect.NORMAL)
+                .build();
+        var request = HttpRequest.newBuilder(url.toURI()).build();
+        var response = client.send(request, HttpResponse.BodyHandlers.ofInputStream());


Not sure I like those vars too much ... 🤔 When building a client or request it's clear to me what the type most likely is, but I'm less sure what the response actually is. Sure, my IDE does show it to, but unfortunately Github does not 😕

I don't think we should be as strict as the presto community, as I think that var does have it's place for local fields in many places...
I probably just need to get used to them 😅

skaldarnar · 2021-08-19T20:12:03Z

src/main/java/org/terasology/launcher/repositories/JenkinsClient.java

@@ -73,8 +82,8 @@ Properties requestProperties(final URL artifactUrl) {
            final Properties properties = new Properties();
            properties.load(inputStream);
            return properties;
-        } catch (IOException e) {
-            e.printStackTrace();
+        } catch (IOException | URISyntaxException | InterruptedException e) {


Why do we catch the InterruptedException here but not in the other places?

oh, because it was used in Optional.map, and the type of thing Optional.map takes cannot throw checked exceptions

skaldarnar · 2021-08-19T20:15:40Z

src/main/java/org/terasology/launcher/util/DownloadUtils.java

        listener.update(0);

-        final HttpURLConnection connection = getConnectedDownloadConnection(downloadURL);
+        var result = getConnectedDownloadConnection(downloadURL);


Hm, I either want an explicit type here, or a better name for getConnectedDownloadConnection 🙈

skaldarnar · 2021-08-19T20:22:30Z

src/test/java/org/terasology/launcher/repositories/JenkinsClientTest.java

+        try (var mockedClientClass = mockStatic(JenkinsClient.class)) {
+            mockedClientClass.when(() -> JenkinsClient.openStream(any())).thenThrow(IOException.class);


Ah, we mock only the static part of JenkinsClient here, right?

just double-checked and I see docs saying "Represents an active mock of a type's static methods," so yes.

keturn · 2021-08-21T10:09:37Z

Launcher follows redirects for Jenkins repo

keturn added 2 commits August 17, 2021 17:29

chore(JenkinsClient): use new HTTP client with better diagnostics

6b1d7db

We get more control of how redirects are handled and more visibility in to the HTTP response if needed for troubleshooting.

chore: typo

72dbf73

chore(DownloadUtils): use new HttpClient for better redirect handling

d543978

This tries to be a minimal change from the previous version, but the mixture of streaming APIs used is not pretty.

keturn commented Aug 19, 2021

View reviewed changes

test(JenkinsClient): throw on any call to openStream

244d242

instead of relying on the URL argument to be used in a specific way

keturn marked this pull request as ready for review August 19, 2021 02:17

Merge branch 'master' into chore/httpClient

0b464ff

skaldarnar approved these changes Aug 19, 2021

View reviewed changes

keturn merged commit 3a1bc1c into master Aug 20, 2021

keturn deleted the chore/httpClient branch August 20, 2021 01:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(JenkinsClient): use new HTTP client with better diagnostics #656

chore(JenkinsClient): use new HTTP client with better diagnostics #656

keturn commented Aug 18, 2021

keturn commented Aug 18, 2021

keturn commented Aug 18, 2021

keturn left a comment

keturn commented Aug 19, 2021

keturn commented Aug 19, 2021

skaldarnar left a comment

skaldarnar Aug 19, 2021

skaldarnar Aug 19, 2021

skaldarnar Aug 19, 2021

skaldarnar Aug 19, 2021

skaldarnar Aug 19, 2021

keturn Aug 20, 2021

skaldarnar Aug 19, 2021

skaldarnar Aug 19, 2021

keturn Aug 20, 2021

keturn commented Aug 21, 2021

		try (var mockedClientClass = mockStatic(JenkinsClient.class)) {
		mockedClientClass.when(() -> JenkinsClient.openStream(any())).thenThrow(IOException.class);

chore(JenkinsClient): use new HTTP client with better diagnostics #656

chore(JenkinsClient): use new HTTP client with better diagnostics #656

Conversation

keturn commented Aug 18, 2021

keturn commented Aug 18, 2021

keturn commented Aug 18, 2021

keturn left a comment

Choose a reason for hiding this comment

keturn commented Aug 19, 2021

InterruptedException

HttpClient

DownloadTask & DownloadUtils.downloadToFile

keturn commented Aug 19, 2021

skaldarnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keturn commented Aug 21, 2021