not thread safe

[jrochkind]

I don't actually use devise/devise-guests, but was reviewing the devise-guest code for ideas on how to upgrade to BL 3.7 in my non-devise app.

I noticed this code:

    def unique_#{mapping}_counter
      @@unique_#{mapping}_counter ||= 0
      @@unique_#{mapping}_counter += 1
    end

https://github.com/cbeer/devise-guests/blob/master/lib/devise-guests/controllers/helpers.rb#L80

I wanted to point out that is not thread safe, for a Rails running under multi-threaded request dispatch. It uses global state, shared between requests, in the @@unique_ variable, but does not synchronize access to this global state from requests, which may be running concurrently in different threads.

Many Rails apps don't do concurrent request dispatching anyway. It requires an app server that can handle concurrent requests (passenger enterprise, puma, maybe a few others in unusual configurations). In Rails3 it also requires you to set config.threadsafe!, although in Rails4 that is on by default.

So it might not matter that it's not thread-safe if hardly anyone runs Rails with concurrent request dispatch anyway, and I probably won't be using this gem myself anyway either, but thought I'd point it out since I noticed it, in case you are concerned.

[cbeer]

Thanks. I think that method is only called from

def guest_#{mapping}_unique_suffix
   Devise.friendly_token + "_" + Time.now.to_i.to_s + "_" + unique_#{mapping}_counter.to_s
end

I suspect we could drop it entirely, and just rely on friendly_token and the current time to provide enough entropy.

[jrochkind]

in my non-devise version, since I don't need to write a generic one that works anywhere, and am in a Rails app, I can just use the already existing Rails-generated session ID. Which, I have no idea how it gets generated, but if it's not unique, is going to cause problems for things other than me.

Not sure what Devise.friendly_token is or if it's supposed to be unique.

Lately I've been really realizing the costs of over-abstraction, sometimes it's easier and actually saves developer time in the long-run to just write simpler code that works in your actual context that you need it in, rather than abstract code that works in any context you can think of. But it's always a trade off sure.

[cbeer]

The Rails session ID ought to be unique. I think there was a "good" reason we couldn't use it here, though. I'll have to check.

[jrochkind]

Only because the way you've structured the code here does not assume Rails, and has no access to Rails controller methods, you've written as a plug-in for devise, and devise does not assume Rails.

[jrochkind]

oh except of course devise does assume rails, but i don't know if you have access to the current rails controller to get session id there or not.

[marcamillion]

Any progress on this? Was this ever fixed?