DataMapper::Resource#run_once isn't thread-safe (and raises in remove_instance_variable)

[dgolombek]

run_once (https://github.com/datamapper/dm-core/blob/master/lib/dm-core/resource.rb#L1208-L1219) is not thread safe. Two callers can each check if the instance variable is set, find it to be false, then each set the instance variable in parallel. This is generally fine -- the method isn't intended to be a real mutex. The problem is that remove_instance_variable in the ensure block will blow up in this situation, since that method raises if the instance variable isn't currently defined.

Given what run_once is attempting to do, I think that simply ignoring errors from remove_instance_variable would be acceptable here, but wanted to get feedback before I submitted a PR with that change.

Thanks
Dave

[tillsc]

This method is an instance method of DataMapper::Resource. So your scenario will only be possible if two or more threads share the same resource instance. Doesn't DataMapper have much more problems if that is the case?

[dgolombek]

I'm not certain -- DataMapper has a stated goal of being thread-safe, but I couldn't find any documentation of how close it actually comes. A few thoughts:

• #run_once is used in dirty? which is a public, read-only method, so it MUST be fixed in that case at least
• Making the same change from two threads simultaneously should always be safe (in my view of thread-safety, anyways)
• I'd hope that non-conflicting changes would be written in parallel (or at least have locking to prevent them from failing)
• Conflicting changes obviously have undefined ordering in the DB, but that's programmer's problem

[tpitale]

@dgolombek PR welcome to use an actual mutex.