So I ran face first into a wall once more. Metaphorically, when coding, I mean. Well, I literally ran into a wall way back when, too, but this time I’m talking about a coding experience… I haven’t decided what’s worse yet.

Anyway, so it all began when I had to download some configuration files in an Android app. This was only a very minor task in the context of said app and I should probably also add that I don’t yet have as much experience in Android coding as I have in iOS programming. I know there are several libraries that provide extensive functionality for making all kinds of HTTP requests and such, but in the end I foolishly decided against including one in the app for such a minor task. Mind you, the idea was very simple: The user clicks a button, and a single, tiny JSON file was downloaded via HTTPS.

Surely, so I reasoned, there was something in the standard libraries allowing me to do that. I had done that often on iOS, piece of cake. And hey, look, there’s HttpsURLConnection, that seems similar to URLConnection on iOS!

As a matter of fact, I had already used that before and for once was pleased it doesn’t come with inherent threading aspects, i.e. you have to process the connection synchronously. In another part of the app I took advantage of that since I had to synchronize a submission (i.e. a POST) to a server with local methods, so I already had a specific thread for that with all the stuff needed to issue calls and such. But I digress.

Here’s the important parts of what I implemented in an AsyncTask I prepared for downloading the configuration:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21

try {
    connection = (HttpsURLConnection) configURL.openConnection();
    connection.setRequestMethod("GET");
    connection.connect();
 
 
    InputStream inputStream = connection.getInputStream(); // this can throw an Exception     
 
    if (inputStream != null) {
        // ... reading the stream into a buffer, then closing it ...
    }
 
} catch (IOException e) {
    // some error logging, the code after the try-catch eventually returns null 
    // and the user sees an error, i.e. indication nothing was loaded. See below
 
} finally {
    if (connection != null) {
        connection.disconnect();
    }
}

Somebody experienced with HttpsURLConnection (or URLConnection in general) may already see where this is going, but for the average developer that shouldn’t look too bad, right? In the context of the app the actual reason for not being able to load the configuration wasn’t really important, as the user won’t be able to do anything besides “try it later” anyway. Either you get the data or you have to live without it, no big deal.

So what’s the problem? Well, to my surprise, if there’s an error due to a specific configuration not being on the server, the app, after quite a while, gave a warning in the debug log about a stream that wasn’t properly closed. Or in other words, there seems to be a potential memory leak. I guess the internal code producing the warning actually closes that stream, but I didn’t test this and there might be a reason for it not to do that. Such a warning should not be ignored, though. The unpredictable timing of that warning (could be over a minute) lets me think it’s actually the garbage collection finding this, but I didn’t investigate further.

In the end (quite a lot) of googling reveals the following:
If the connection can be established, but the server answers with anything that has an HTTP status code above 400, getInputStream() throws an exception. That already seemed odd to me, but I could at least in part understand that (see below on how I would design this). I did notice that before and was aware of my code basically ignoring that, but that was because I didn’t need any further info. Besides, the exception itself is not sufficient to fully get the error anyways.

What HttpsURLConnection (and HttpURLConnection, btw) does is that in case of an error it creates and opens another InputStream that you have to get with getErrorStream().

In my humble opinion, that violates a cardinal design rule: Never allocate memory you do not clean up yourself if possible! If you create an object for a caller, tell them explicitly about it and inform them (in the documentation if your language’s syntax doesn’t provide a better means) that it has to free that object.

In Java that is admittedly a rare thing due to garbage collection, but streams, sockets, etc. are an unfortunate exception (pun kind of intended here). Note that the InputStream does not need to be open()ed in my code above either (there might be technical reasons for this), but at least I explicitly got it with getInputStream().

I rarely dare to criticize a standard library’s code, as I am usually assuming the people writing them have a fair bit of experience and know what they’re doing. Besides, older standard libraries have lots of users, so it’s unlikely that a bug in something relatively mundane has not been found. It’s much more likely that I did something wrong, so I doubted myself quite a bit.

In this case, though, the way its designed is just stupid. It forces you to write useless try-catch blocks, ultimately blowing up your code.
Now, you can put everything into clever helper methods, bouncing around exceptions and make everything look very elegant, but here’s what this ultimately results in:

• You try to get an input stream to read whatever it is that’s on your server.
• Handle an exception, but don’t think that you can get anything useful from it. “Handle” means instead:
  • Definitely get another input stream, one that’s specifically created to read the error the sever gives you. Regardless of whether your behavior is the same for all errors, take the thing! Take it!
  • Oh, obviously you have to be prepared for an exception again, just, you know, for the case that the connection attempt really results in an error

Now take what I marked bold in the third bullet point and let that sink in. Yes, in spite of the first exception, I am still reading from the server. You know why? Because an HTTP request resulting in you getting an HTTP status code indicating an error, it’s not a connection error. “Error” in this context means “you made a mistake, dear client and I cannot help you”. Double-U Tea Eff!

An HttpsURLConnection should not throw an exception when it actually manages to speak to the server. Even a 4xx response is a valid response from the perspective of my own local network stack.

Even with the weird design choice to have a separate stream instance, why is this already created, apparently opened and then simply expected to be taken by me, the caller, to close? Wouldn’t it be possible to do that lazily?

Anyways, I guess from this it’s pretty clear what I would do when designing this. I’d only throw an exception if my own network stack was broken somehow and I couldn’t even make the connection to the server or it broke down during loading. If I get any kind of response, I’d deliver it in the same damn input stream. It’s no magic, or even unreasonable to expect the consumer of that stream (who will be well aware that it was created and needs to be properly closed at this point) to interpret the status code and act accordingly.

Obviously I am talking about an improvement on this in general. A completely different approach is to register a delegate of some sort (or even a lambda of some sorts) to be informed. That’s how iOS does it. The callback gets an error and the response simply as parameters passed to the registered callback.