Mark AddTypeHandlerImpl as obsolete and prevent lost updates via AddTypeHandler

[mgravell]

AddTypeHandlerImpl was never intended to be public; oops

also drive-by fix to prevent lost updates via AddTypeHandler

• mark AddTypeHandlerImpl as obsolete
• syncronize type-handler mutates, to prevent lost writes

fix #1815

alternate to #2107

[mgravell]

(CI fails: appveyor keep changing the pgsql setup; trying to fix)

[billrob]

Still not 100% perfect, but 99% on an admin / housekeeping operation will be fine.

[mgravell]

Still not 100% perfect, but 99% on an admin / housekeeping operation will be fine.

what concrete scenario are you concerned about?

[mgravell]

giving up on fighting appveyor - summoning the one, the only, @NickCraver - any clue? appveyor keep changing the base image; I fixed once here, but can't get to work now

[billrob]

Probably more a "not how I would do it" 1%. The iteration when newing the dictionary is still potentially an exception. At least it will throw an exception rather than build a non-desirable dictionary cache. But verily, if someone is running multi-threading add/reset/remove type handlers we could just tell them to not do it.

I don't see that currently possible with the current code base, but more future proofing, because in 2 years will we remember this discussion? lol

[mgravell]

The iteration when newing the dictionary is still potentially an exception.

I don't see how:

1. we never mutate the dictionary once assigned, except for in the .cctor (that's what clone controls)
2. it doesn't matter because of 1, but we're inside the lock for this iterator, so we know nobody is doing anything except reading

So : in what scenario can we compete? Happy to consider a concrete scenario, but : I don't see it currently.

As for the 2 years thing: we might not remember this specific discussion in the future, but yes: we will know that this is static state that needs concurrency consideration.

[billrob]

Any potential issues does not exist currently in the code base and this PR is threaded sound. I'm confident this currently fixes the known threading issues and you should push it. (appveyor issues aside)

Larger story here, I want Dapper to survive EF consuming this project as they often consume popular projects. Should we consider a .UseDapper(with type handler registration) that can internally register this static dictionary as a singleton with DI? That would eliminate all these redo scenarios we potentially could face in the future and fit more inline with .net core direction.

[mgravell]

I'm not sure that's the best option for configuration in this case. Over in the AOT side, we allow declarative and contextual configuration via attributes, which allows config to be as granular or global as desired without the need for additional registration or service tracking.

[billrob]

I had a hard time doing static typemap configuration as Dapper and .net evolved regarding testing. AOT might be a great solution, but it is so far from on-the-ground developers just wanting things to work.

The dotnet .UseXXX has taken off. Dapper is infinitely more configurable than the current EF implementation mimicking Dapper's performance. I might be myopic here, but why shouldn't we want a startup configuration that handles application specific type mappings because almost all scenarios don't involve changing type map configurations while your application is running.

[mgravell]

Because you can already do that today without requiring a new API. Service registration kinda needs a service state to be "proper" (not just static state), and the existing Dapper bits lack that. As for AOT: you don't need to be deploying via AOT for Dapper's AOT bits to be useful - the aim there is to remove all the reflection code from the critical path.

[NickCraver]

Postgres fixed up in #2130 - the docs are just totally missing past Postgresql 13, so you're not crazy. Even more reason we need to get off AppVeyor.