search

f1ana / Nominatim.API

Library for utilizing geocoding (forward and reverse), in addition to address lookups, with the Nominatim HTTP API. Targets .NET 6 and .NET Standard 2.0.
https://www.nuget.org/packages/Nominatim.API
MIT License
50 stars 27 forks source link

Unnecessary synchronisation #1

Closed Wufflez closed 6 years ago

Wufflez commented 6 years ago

A colleague of mine recently had issues using this API. He is not familiar with modern .NET practices (like async/await) and as he intends to use the API in a legacy application anyway he stuck to using the API synchronously in a similar style to the unit tests. He ran into issues with the API deadlocking, which could only be resolved by firing off calls to the API on a background thread, which prompted him to ask me to take a look.

I quickly noticed that all async continuations internal to the library are posted back to a captured synchronisation context from the thread calling into the API (this is a result of the default behaviour of 'await' if you are not aware of it). This is of course not required as the library is just deserialising JSON, building new objects to return, etc. and has no need to synchronise anything. In fact, it also hurts the performance of the library because it prevents responses from being processed in parallel and of course the overhead of posting all work to another (potentially busy) thread.

To give an example, if the user of the API is writing a WinForms application, there are two possible outcomes depending on how they call the API.

Asynchronously (e.g. await): The library behaves correctly but the performance is still very much sub optimal due to the UI thread being used internally for everything by the library. Synchronously (e.g. response.Result): The application will hang indefinitely.

A big improvement to the library would be to remove the synchronisation by configuring the awaits not to continue on the captured synchronisation context by extending awaited calls with 'ConfigureAwait(false)'. This will not only improve general performance but also help prevent developers using a synchronous workflow from shooting themselves in the foot!

f1ana commented 6 years ago

Hi Adam,

Thanks for bringing up this issue! Much appreciated. Do you have a PR that you would like to submit? If not, I can push out a fix when I have a few free moments.

Thanks, Chris

Wufflez commented 6 years ago

No problem. I don't have one now (I'm at work) but I'm more than happy to submit one as soon as I get a few minutes to make the change.

f1ana commented 6 years ago

Hi, I noticed that you had forked and committed a fix. Are you all set to issue a PR?

Wufflez commented 6 years ago

Yes, very sorry. PR issused! It's just been one of those weeks for me -_-

f1ana commented 6 years ago

Merged using #2

f1ana commented 6 years ago

Hi @Wufflez, just wanted to let you know that I've pushed a new Nuget package as of a few minutes ago. So sorry for the delay, but thanks once more for your contribution!

Wufflez commented 6 years ago

Hi,

Thanks for the update. It is not urgent, so no rush. Thanks for the library :)

Cheers,

Adam

On Wed, May 2, 2018 at 3:37 PM, Christopher Flanagan < notifications@github.com> wrote:

Hi @Wufflez https://github.com/Wufflez, just wanted to let you know that I've pushed a new Nuget package as of a few minutes ago. So sorry for the delay, but thanks once more for your contribution!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/f1ana/Nominatim.API/issues/1#issuecomment-386000224, or mute the thread https://github.com/notifications/unsubscribe-auth/AN5AbF0_OPHeaPatXauUgrOlJBYgDN3Iks5tucSzgaJpZM4TgJQU .