Use of .Result and async native code

[egbertn]

I had to fork your interesting api because.net 3+ will fail after a few requests.

1) abstract class HttpClient is using IDisposable pattern, including the 'suppress GC' issue. This should not be followed today anymore. Just remove the IDisposable for better GC. 2) Combining .Result and .ContinueWith and 'async + await' is subscribing to a disaster. Use google search to see if this is so.

I had to remove those features and now it is stable. (Using .NET 5.0 core)

[vivet]

Hi I will look into it

[vivet]

I took a look at your fork. Seems you changed the non-async Query to be async. Have you tried using QueryAsync, instead?

Also, can you please link to where you have read not to use IDisposable.

[egbertn]

Thanks for your response my friend, I restored the 'sync' method to what it should be. I tested it. But not on .Net 4.6+.

I get what you mean, but the async code at places used .Result at some places. Thats in my experience subscribing for weird issues or a disaster. I got an internal CLR exception So I even could not stop nicely the process. See last issue. https://github.com/dotnet/runtime/issues/13590

So, what I did is a 'quick fix' I did not maintain your original design or run tests, or try .NET 4.6 etc. But defintely in-variables never should be 'disposed' like here using (httpResponse) { } So, now I got your nice component stable and b.t.w. the '350 ms' google delay on paging is necessary. Google probably uses Elastic Search so their 'shards' are not in sync.

THe thumb of rule is, never mix async meant to be async with sync. Also, there is a lot of Disposable variables going on in the return values of HttpEngine. I would strive to handle the response at one place.

About the IDisposable. It may not have been the reason for crashing my process so this more is my personal choice, to make classes 'sealed' instead. But abstract classes cannot be. Since the topic is hard to prove but in my experience lead to real leaks, I just avoid such code or rewrite it to be sealed. https://softwareengineering.stackexchange.com/questions/251274/can-gc-suppressfinalize-cause-performance-problems

https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1816 I would make HttpClient part of the constructor logic, so the component can be registered using IoC and the IoC framework can deal with cleaningup. :)

[vivet]

Hi I will try to address it all :-)

1. Using .Result is horrible, I agree. BUT, there are situations where applications can't use async pattern, and needs have a non-async query method. That's why it's included - backward compatibility. As far as I know there is no other way. use QueryAsync(...) whenever possible.

2. Regarding dispose then it's correct to dispose the Httpresponse when I am done with it. Not sure why you would suggest otherwise.

3. The Google paging you will need to deal with. I don´t want to delay the request, as it would also delay the first request. This is a consumer responsiblity.

4. Regarding GC and disposable pattern for the HttpEngine, then this is correct too. The HttpClient should not be disposed on each call. It should be kept and disposed with the application, which is exactly what the code does. I could have omitted the implementation of IDisposable, but that allows the consumer to control when its disposed and GC'ed. If you don't do anything, it will just be disposed when the application shuts down. If your read your own links a bit more careful you will see the I am not breaking the MS rule, same goes for the stackexchange link.

5. There is no reason for making the HttpClient injectable through the constructor, it's kind of the idea behind the facade pattern. So I won't do that.

6. I noticed in your fork, that you removed ConfigureAwait(false). Don't do that, cause it will give you problems with ASP.NET

Conclusion: In general it seems to me, that you got yourself into a problem, because you didn't use the async version of query. I don't see any arguments that has convinced me otherwise, and based on that I won't change the code. I suggest you try the async version of query.

[egbertn]

I got myself into trouble, not by using the sync version, but the async version. It leads to a hard crash, i fixed it. You don't need to be convinced by me. Need to get things working, and now it works.

Using GC.Suppress will cause the normal GC to delay resources to be freed. I dealt with this in the fork. So, http client has it's own using scope within the function. We don't need to rely on GC to free the resources, that way.

At least one thing you could 'admit' :) using (httpresponse) while response is an in parameter is not correct. The calling function is responsible for that. Not the consuming function. That also got me into trouble. It caused an 'object already disposed' exception.

You should dispose, when an instance implements it of course. Again, not sure why the horrible crashes exactly which line caused it, but my fork is now stable. About not using ConfigureAwait(false) causing problems in ASP.NET. Never heard of that before.

[vivet]

Fist, the async query version doesn't use .Result :-) so not sure what you are doing. Second, read the links your reference more careful. The response is parameter into a private method.

Consider for a moment that a lot of users are using this library, including myself, having 50 request to google per second, for dispatching engines. If you feel more comfortable using your fork, then do that, but you lack knowledge about this topic.

[egbertn]

Consider this an first issue on .NET 5.0. I did not hack at all, just a normal call to Google. Second call using Nearby Places led to the crash. Of course, I lack knowledge in this topic, but for real, I had to fix the issue. I hope you don't think I make this info up just to waste some of your time.

[vivet]

I don't think you making this up :-) I am just arguing. Can you paste a snippet of your code, then I will gladly assist to see if the issue is there. I haven't used .NET 5.0 yet with GoogleApi. I thought you where referencing 3.1. Within the next week I will do some net50 tests.

But please paste a snippet demonstrating what your are doing, then I will run a test on that too.

Also, there might be changes to how the HttpClient should be handled in net50.

[vivet]

Hi I will close this issue, as you haven't replied. I hope it works for you, otherwise you can use the fork you made.

[egbertn]

No problem.

[HttpGet("NearByPlaces")] [ProducesResponseType( typeof(IEnumerable), 200)] public async Task NearByPlaces([FromQuery] double latitude, [FromQuery] double longitude, [FromQuery] IEnumerable category= null, [FromQuery] int? radius = null, [FromQuery] string name = null, [FromQuery] string token = null) { try { var data = await _googlePlacesClient.NearByPlaces(new LatLong { Latitude = latitude, Longitude = longitude }, category, radius: radius ?? 500, name, token); return Ok(data); } catch(Exception ex) { _logger.LogError("NearByPlaces failed with {0}", ex.Message); } return StatusCode(StatusCodes.Status500InternalServerError); }

// some non relevant code is removed for clarity

public async Task NearByPlaces( LatLong latlong, IEnumerable categories, int radius = 500, string name = null, string token = null) { string[] searchTypes = default; if (categories?.Any() ?? false) { searchTypes = _googleSettings.DefaultSearchTypes.Where(w => categories.Contains(w.Category)).SelectMany(s => s.Types).ToArray(); } //pageToken to search next 20 var placesList = new Dictionary<string, Place>(); var req = new PlacesNearBySearchRequest() { Key = _googleSettings.MapsApiKey, Radius = radius, //OpenNow = true, PageToken = token, Keyword = name, Location = new GoogleApi.Entities.Places.Search.NearBy.Request.Location(latlong.Latitude, latlong.Longitude), Language = GoogleApi.Entities.Common.Enums.Language.English | GoogleApi.Entities.Common.Enums.Language.Arabic, //Type = (GoogleApi.Entities.Places.Search.Common.Enums.SearchPlaceType)searchTypes.First(), Rankby = Ranking.Prominence }; var key = $"places_v2:{req.Key}{req.Radius}:{latlong}";// var nextPageToken = default(string);

            try
            {
                var doSearch = await GoogleApi.GooglePlaces.NearBySearch.QueryAsync(req);
                var searchResult = default(IEnumerable<Place>);
            //  System.Diagnostics.Trace.TraceInformation("from Google NearbySearch {0}", doSearch.RawJson);
                if (doSearch.Status == GoogleApi.Entities.Common.Enums.Status.Ok)
                {
                    searchResult = doSearch.Results.Select(s =>
                    new Place
                    {
                        Id = s.PlaceId,
                        Vicinity = s.Vicinity,
                        Name = s.Name,
                        OpenNow = s.OpeningHours?.OpenNow,
                        Rating = s.Rating,
                        NumberofRatings = s.UserRatingsTotal,
                        IconUrl = s.IconUrl,
                        Photos = s.Photos?.Select(s => new Photo
                        {
                            Height = s.Height,
                            Width = s.Width,
                            PhotoReference = s.PhotoReference
                        }).ToArray(),
                        Types = s.Types.Select(x => x.ToString()).ToArray(),
                        Location = new Shared.Models.Location
                        {
                            Address = s.Geometry.Location.Address,
                            Latitude = s.Geometry.Location.Latitude,
                            Longitude = s.Geometry.Location.Longitude
                        }
                    }).ToArray();
                    nextPageToken = doSearch.NextPageToken;
                    //return  searchResult;
                }
                else //status anything else
                {
                    _logger.LogWarning("NearbyPlaces issued error with status {0} and msg {1}", doSearch.Status, doSearch.ErrorMessage);
                }
            }

    }

[egbertn]

Note: It seems that it happens especially during debugging.

[vivet]

I will try your code.

Note: Debugging is generally a bad idea. Avoid that as much as possible.

[egbertn]

lool...

[vivet]

haha. I wasn't kidding. Debugging is evil :-) I will try out your code.

[vivet]

I tried out your logic, and executed nearby places 10 times, using the NextPageToken. No issues. In the code above, I don't see any apparent problems, but i noticed _googlePlacesClient, which to me seems that you made a wrapper around GoogleApi. That might cause the problem, especially if you have used dependency injection, and made it scoped.

I suggest you try to shave as much code away as possible, and just invoke GoogleApi.GooglePlaces.NearBySearch.QueryAsync(req) in your endpoint, until you see it working.

Hope it helps.

[egbertn]

Thanks for trying to repro, but I use Transient scope, that is the most safe one. Currently I got it working with the edits I made. Just wanted to support your project. Also, it was not related to nextpage token, but just the one having (only) Location+Radius as arguments, it directly crashed. Good luck.

[vivet]

Good luck to you too