search

jonwagner / Insight.Database

Fast, lightweight .NET micro-ORM
Other
861 stars 145 forks source link

Added a lock around code generation #412

Closed resnikb closed 4 years ago

resnikb commented 5 years ago

Description

Code generation in Insight is not thread-safe, as ModelBuilder and related classes are not thread-safe, and concurrent execution can cause unpredictable results. This is one of the possible causes for recent bug reports involving missing methods in Insight-generated classes (#407 and #408)

Checklist

Please make sure your pull request fulfills the following requirements:

N/A, as deterministic tests would be hard/impossible to create.

Type

This pull request includes what type of changes?

Breaking Changes

Does this pull request introduce any breaking changes?

Any other comment

(n/a)

nwpz commented 5 years ago

Is this in the pipeline to get reviewed/merged? I have encountered the issue described in #407 and #408 and am hopeful this resolves it.

jonwagner commented 5 years ago

I don’t have any direct evidence that it solves the problems so I haven’t kicked off a formal build. Can you do a local build of the branch and test it? Or should I work on a teat build?

I should be able to get to a production build during the US holiday week.

On Nov 22, 2019, at 2:12 PM, nwp notifications@github.com wrote:



Is this in the pipeline to get reviewed/merged? I have encountered the issue described in #407https://github.com/jonwagner/Insight.Database/issues/407 and #408https://github.com/jonwagner/Insight.Database/issues/408 and am hopeful this resolves it.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHubhttps://github.com/jonwagner/Insight.Database/pull/412?email_source=notifications&email_token=AAMTO5AGETGFRITVOZQID6TQVAVKVA5CNFSM4JKWCX6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEE6SYHQ#issuecomment-557657118, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAMTO5CL6MJORZ2K53Q27QTQVAVKVANCNFSM4JKWCX6A.

nwpz commented 5 years ago

I'm trying a local build, will let you know if I see the issue with this change. It's been hard to pinpoint. I've seen this sporadically for a about a year now in both .net core 2.1 and 2.2 when using insight.database. Will let you know if i see it again with this change applied.

resnikb commented 5 years ago

@jonwagner This one is hard to test and verify that it relates to the reported issues. The reason I went looking for threading issues in the first place were the commonalities in the issues:

This smelled of a race condition to me, so I looked at the generator code, and realised that it could be executing concurrently, which would likely have similar symptoms to the ones described, as the involved framework types are not thread-safe.

jonwagner commented 4 years ago

Good catch @resnikb !

I was a little worried about performance issues converting it to single-threaded, but it's only for interface generation. There should be minimal impact switching to single-threaded.

jonwagner commented 4 years ago

I think I'll go with a variant of this code. Your PR puts the lock around the construction of the objects, which will single-thread connection creation. I'll refactor that method and just put the lock around the interface generation.

jonwagner commented 4 years ago

This fix (with slightly better locking) is now in 6.2.11.

Thanks @resnikb

resnikb commented 4 years ago

@jonwagner Thanks for looking into it.

However, I think your code change is not enough. The reason is that the delegate supplied to ConcurrentDictionary.GetOrAdd can be called concurrently for the same type - in that case the current code will try to define the same type twice, resulting in a different exception.

This is the reason why in my PR I used double-checked locking, to ensure that generation can only run once for a given type. Also, please note that I don't have a lock around object creation, only around the call to CreateImplementorOf and TryAdd to ensure atomicity of the operation.

jonwagner commented 4 years ago

Yes, you’re right that GetOrAdd calls the delegate outside of the locks and only locks the collection at add time.

If the interface generation code is called simultaneously for the same interface, I think the worst case is that two different classes are created. What error do you see it throwing?

resnikb commented 4 years ago

@jonwagner Ah, true, my bad - I missed the fact that type names are generated using a guid suffix. You're right that in case of concurrent executions there won't be any exceptions, and the current code will work as expected.

The only drawback of the current implementation that I see, is the potential to generate multiple implementing types for the same interface - there could be more than two, depending on how many concurrent executions of the GetOrAdd delegate start before the dictionary is updated, as each thread that starts executing the delegate will create one. This may be an issue for some applications, as it can add a lot of wait time in highly concurrent scenarios. For example, with N threads executing the delegate concurrently, the last thread to enter the lock will take N x (time to generate implementation type) to retrieve the constructor delegate.

On one hand, I agree that having more than a few threads there is an edge case, but on the other, it is relatively straightforward to prevent this behavior. What are your thoughts?

jonwagner commented 4 years ago

I’d rather write less locking and thread management code. Too easy to get wrong (see my first attempt...)

If we’re down to creating multiple implementation classes, only one would get used, so it’s more of a resource bloat/startup issue.

It’s probably worth putting the try/get call around it to totally eliminate that case. I’ll take a look at your commit and pull that in.

On Nov 29, 2019, at 10:24 PM, Bojan Resnik notifications@github.com wrote:



@jonwagnerhttps://github.com/jonwagner Ah, true, my bad - I missed the fact that type names are generated using a guid suffix. You're right that in case of concurrent executions there won't be any exceptions, and the current code will work as expected.

The only drawback of the current implementation that I see, is the potential to generate multiple implementing types for the same interface - there could be more than two, depending on how many concurrent executions of the GetOrAdd delegate start before the dictionary is updated, as each thread that starts executing the delegate will create one. This may be an issue for some applications, as it can add a lot of wait time in highly concurrent scenarios. For example, with N threads executing the delegate concurrently, the last thread to enter the lock will take N x (time to generate implementation type) to retrieve the factory delegate.

On one hand, I agree that having more than a few threads there is an edge case, but on the other, it is relatively straightforward to prevent this behavior. What are your thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/jonwagner/Insight.Database/pull/412?email_source=notifications&email_token=AAMTO5GHBKYLQECOFTQPY5DQWHMFRA5CNFSM4JKWCX6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFPYNQA#issuecomment-559908544, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAMTO5AX4NMJNOHTGU2VOM3QWHMFRANCNFSM4JKWCX6A.