Skip to content

get_session is not thread-safe #19

Closed
emou opened this Issue Sep 13, 2013 · 10 comments

6 participants

@emou
emou commented Sep 13, 2013

This code is not thread-safe:

  def get_session(env, sid)
      ActiveRecord::Base.logger.quietly do

because ActiveRecord::Base.logger.quietly is not thread-safe.

See rails/rails#11954

The resulting issue is that this can break stdout in a multi-threaded process.

The real-world issue we're hitting is that after upgrading to rails 4, when we run our specs the standard output from rspec disappears, and the test results are not visible.

This happens when there are multiple requests hitting rack, which spawns a thread for each one. These threads in turn call 'get_session' concurrently, and the stdout is being improperly "restored" to '/dev/null' due to the thread unsafety here.

Not sure if this should be fixed here or possibly in active_support (again, rails/rails#11954).

@tommeier
tommeier commented Dec 3, 2013

I'm also experiencing this, took forever to narrow down, glad to see someone else is getting this too!

@nathany
nathany commented Mar 18, 2014

From rails/rails#14031. I've spent a few man-days tracking down a production logging issue which turns out to be this.

I have a demo app for the issue. https://github.com/GetJobber/logging_example

If I comment out the 3 calls to logger.quietly the issue is solved in the example app.

Looking for the alternative to logger.quietly or if it can just be removed.

@harikrishnan83

I am facing this issue too with Puma on MRI. I am trying out below Gist by @kares.
https://gist.github.com/kares/9374772

@nathany
nathany commented Mar 28, 2014

@harikrishnan83 Or you can use this fork #22 rather than monkey patching silence.

@harikrishnan83

Thanks @nathany. I will try it.

@tommeier

I'm using this as an initialiser thats doing the job (this is not with puma though, we only face issues with multiple threaded tests etc): https://gist.github.com/tommeier/9845210

@tommeier

Also, the concensus on rails itself is to remove these methods entirely: rails/rails#13139 but I wasn't sure at the time the best way to resolve tests as it is being used by multiple 'sub' projects and no shared logic seems to exist yet.

@sikachu
Ruby on Rails member
sikachu commented Jan 16, 2015

I've implemented the fix for both thread safety issue and quietly deprecation and pushed out a prelease gem. Would you mind testing out 0.1.1.pre and report back if you still run into any issue?

Thanks!

@sikachu
Ruby on Rails member
sikachu commented Jan 23, 2015

I'm going to release 0.1.1 which should contain the fix. If this is still an issue, please let me know so I will reopen the issue. Thank you for your patience.

@sikachu sikachu closed this Jan 23, 2015
@alexandermalfait

Thanks for the fix! This issue was completely crashing my JRuby application.
All requests would fail with this stack trace:

Errno::EBADF (Bad file descriptor - Bad file descriptor):
  org/jruby/RubyIO.java:2097:in `close'
  activesupport (4.1.4) lib/active_support/core_ext/kernel/reporting.rb:51:in `silence_stream'
  activesupport (4.1.4) lib/active_support/core_ext/kernel/reporting.rb:104:in `quietly'
  activerecord-session_store (0.1.0) lib/action_dispatch/session/active_record_store.rb:77:in `set_session'
  rack (1.5.2) lib/rack/session/abstract/id.rb:342:in `commit_session'
  rack (1.5.2) lib/rack/session/abstract/id.rb:226:in `context'
  rack (1.5.2) lib/rack/session/abstract/id.rb:220:in `call'

Updating activerecord-session_store to ~> 0.1.1 fixes the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.