RFR: 2182: Throw UncheckedRestException on empty GET responses [v2]

[Erik Joelsson]

On Wed, 28 Feb 2024 19:06:02 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> A very common type of SEVERE logging message, which we get notifications for as admins, is this:
>> 
>> Exception during periodic items checking: Cannot invoke "org.openjdk.skara.json.JSONArray.stream()" because the return value of "org.openjdk.skara.json.JSONValue.asArray()" is null
>> 
>> There are some other kinds of instances where the NPE isn't providing any information, but that are likely caused by the same thing. From what I can tell, what's happening is that a REST GET call (typically to GitHub) returns an empty string in the response body. Our JSON parser translates this to a JSONNull and we subsequently try to treat it as a JSONArray and get the NPE when trying to call `stream()` on it. This typically happens a few times per day across all the bots. Ultimately I think this is a bug with GitHub, but we need to deal with it in a better way.
>> 
>> My proposed solution is to add an option to RestRequest, `failOnEmptyResponse`, that defaults to true for GET requests but otherwise false. If set and the response body is empty, an UncheckedRestException is thrown. This exception is already handled as just a warning and we are collecting metrics for it to sound alarms if we get a lot of them.
>> 
>> Investigation and experimentation with this patch in our staging environment showed that these are (at least primarily) a mix of 302 and 304 responses. The 302 from JBS with a redirect to a maintenance page and 304 from GitHub for an unknown reason. We could have tried to handle every response code, or made >=300 errors, but for now I think the current approach is more straightforward and more likely to catch the problems we are specifically seeing. We are still logging the response codes so if we see other patterns in the future, we can adapt.
>
> Erik Joelsson has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Only include the redirect in the RestException message if Location header is present
>  - Include Location in RestException message
>  - Improve logging of RestException

Investigation and experimentation with this patch in our staging environment showed that these are (at least primarily) a mix of 302 and 304 responses. The 302 from JBS with a redirect to a maintenance page and 304 from GitHub for an unknown reason. We could have tried to handle every response code, or made >=300 errors, but for now I think the current approach is more straightforward and more likely to catch the problems we are specifically seeing. We are still logging the response codes so if we see other patterns in the future, we can adapt.

-------------

PR Comment: https://git.openjdk.org/skara/pull/1614#issuecomment-1969664628