1

I'm using a httpclient getasync call. And i'm hitting it multiple times. And sometimes it is giving this kind of exception, which is not falling under try catch block.

    public async Task<(string exception, ObservableCollection<AddressInfo> autoCompleteInfo)> GetGooglePlacesAsync(string input)
    {
        try
        {
            using (var client = new HttpClient())
            {
                var response = await client.GetStringAsync(string.Format(GooglePlacesApi,
                GooglePlacesApiKey, WebUtility.UrlEncode(input)));

                PlacesLocationPredictions predictionList = JsonConvert.DeserializeObject<PlacesLocationPredictions>(response);

                if (predictionList.Status == "OK")
                {
                    LocationList.Clear();

                    if (predictionList.Predictions.Count > 0)
                    {
                        foreach (Prediction prediction in predictionList.Predictions)
                        {
                            LocationList.Add(new AddressInfo
                            {
                                Address = prediction.Description,
                                PlaceID = prediction.Place_Id
                            });
                        }
                    }
                }
                else
                {
                    return (predictionList.Status, null);
                }
            }
        }
        catch (Exception ex)
        {
            return (ex.Message, null);
        }
        return (null, LocationList);
    }
San
  • 65
  • 7
  • is this asp.net core? – jazb May 10 '19 at 06:46
  • Which line in the code throws the exception? – Chetan May 10 '19 at 06:46
  • Debug your code, find out which line throws and fix the bug by checking for null. That catch block is a *very* bad idea too. Not only does it *hide* the error, it loses the exception information. An Exception contains the exact location where the error occured and the call stack that shows *which* method threw and the chain of calls that lead to it. – Panagiotis Kanavos May 10 '19 at 06:49
  • @JohnB that's not the case and not related to the error anyway. Disposing *may* cause scalability issues if there's a lot of traffic but that won't cause an NRE. – Panagiotis Kanavos May 10 '19 at 06:50
  • I agree with Panagiotis Kanavos a Task will already store an exception (in its Task.Exception property) if it occurs including callstack and everything else. There is no need for your code to return an exception message, except to make the usage of code awkward and cumbersome. Just return a Task> and remove your try-catch altogether. – ckuri May 10 '19 at 06:53
  • The duplicate explains how to troubleshoot an NRE by debugging the code, checking the exception etc. The code itself can generate nulls in several points. For example, is `input` null? What happens if `GetAsync` fails? The code *never* checks its contents which means `JsonConvert.DeserializeObject` in turn can fail or return null. This means that `predictionList.Status` will throw an NRE if the response wasn't a JSON object – Panagiotis Kanavos May 10 '19 at 06:54
  • As for `predictionList.Status` why is it used? An HTTP/REST API is supposed to return a 4xx/5xx error response should it fail. For `predictionList.Status` to return anything, the HTTP call would have to succeed and return a well formed JSON string containing `Status` as a top-level dictionary element. – Panagiotis Kanavos May 10 '19 at 06:56
  • The list construction code can fail if `LocationList` is null, or if `predictionList.Predictions` is null. It's overcomplicated too. It can be replaced by a single `LocationList= predictionList.Predictions.Select(prediction =>new AddressInfo {Address = prediction.Description,PlaceID = prediction.Place_Id}).ToList();`. This will actually be faster and use less memory. No need to check the count, no need to clear the original list. – Panagiotis Kanavos May 10 '19 at 06:59
  • You can even handle null collections and variables if you use `?.` and `??`, eg `predictionList?.Predictions?.Select(...)?.ToList() ?? new List<..>()`. The nullsafe `?.` operator will return null if its argument is null. The null-coalescing operator `??` will replace a null on the left with the default value on the right, an empty list – Panagiotis Kanavos May 10 '19 at 07:03
  • 1
    Furthermore, using globals to return results is a bad idea. A method should return its results, not store them in a field or property. This makes it far easier to make the method thread-safe and exception-safe. *This* method is neither - it can easily fail due to threading or exception issues. What happens if it's called *twice*? What happens if someone tried to read `LocationList` in the meantime? What happens if an exception is thrown leaving `LocationList` half-filled? That's why returning status codes instead of throwing is **bad** – Panagiotis Kanavos May 10 '19 at 07:07
  • Finally, tuple return types in Go and C# are meant to return either a result or an error as an alternative to throwing in case of an error but not an outright programming bug. Returning an error result is appropriate when the HTTP service returns a failure code. An NRE though means you no longer know what the code did or what state the program is in. – Panagiotis Kanavos May 10 '19 at 07:12
  • Your `LocationList` is accessed from multiple threads, causing the NRE to be thrown from `List` (or whatever it is) internally. Make it a local variable. – CodeCaster May 10 '19 at 07:15
  • @CodeCaster. Thanks for the hint. It helped a lot to resolve the issue :) – San May 11 '19 at 05:55

0 Answers0