Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lockdown dalli to 3.0.6 while we address breaking changes in 3.1.0 #21595

Merged
merged 1 commit into from
Dec 6, 2021

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Dec 6, 2021

Fixes #21593

Dalli 3.1.0 was released and had the following changes from 3.0.6:
petergoldstein/dalli@v3.0.6...v3.1.0

Among these these changes:

  • destroy_session was renamed to delete_session
  • with_block is now with_dalli_client

This is important because we have monkey patches on these private methods to
ensure we can safely load code in threads from within them. When these patches
aren't applied, we can get deadlocks.

Additionally, request.session_options[:id] is now a Rack::Session::SessionId
object that can't be cast in find_by calls so we may need to make this change in
user:

-    sessions << Session.find_or_create_by(:session_id => session_id)
+    sessions << Session.find_or_create_by(:session_id => session_id.private_id)

Related to rails/activerecord-session_store#154

Note, even with local changes to account for the renames and this change in the
session_id, some pages were still hung so we'll just lock down to 3.0.6 until we
can resolve them.

Fixes ManageIQ#21593

Dalli 3.1.0 was released and had the following changes from 3.0.6:
petergoldstein/dalli@v3.0.6...v3.1.0

Among these these changes:
* destroy_session was renamed to delete_session
* with_block is now with_dalli_client

This is important because we have monkey patches on these private methods to
ensure we can safely load code in threads from within them.  When these patches
aren't applied, we can get deadlocks.

Additionally, request.session_options[:id] is now a Rack::Session::SessionId
object that can't be cast in find_by calls so we may need to make this change in
user:

-    sessions << Session.find_or_create_by(:session_id => session_id)
+    sessions << Session.find_or_create_by(:session_id => session_id.private_id)

Related to https://github.com/rails/activerecord-session_store/issues ManageIQ#154

Note, even with local changes to account for the renames and this change in the
session_id, some pages were still hung so we'll just lock down to 3.0.6 until we
can resolve them.
@miq-bot
Copy link
Member

miq-bot commented Dec 6, 2021

Checked commit jrafanie@b3e4c69 with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@bdunne bdunne merged commit d943a50 into ManageIQ:master Dec 6, 2021
@bdunne bdunne self-assigned this Dec 6, 2021
@jrafanie jrafanie deleted the lock_down_dalli branch December 6, 2021 16:53
@Fryguy
Copy link
Member

Fryguy commented Dec 7, 2021

@jrafanie A conflict occurred during the backport of this pull request to morphy.

If this pull request is based on another pull request that has not been marked for backport, add the appropriate labels to the other pull request. Otherwise, please create a new pull request direct to the morphy branch in order to resolve this.

Conflict details:

diff --cc Gemfile
index e790e7c7a1,2eb2ab589b..0000000000
--- a/Gemfile
+++ b/Gemfile
@@@ -30,8 -30,10 +30,12 @@@ gem "bcrypt"
  gem "bundler",                          "~> 2.1", ">= 2.1.4", "!= 2.2.10", :require => false
  gem "byebug",                                                :require => false
  gem "color",                            "~>1.8"
 -gem "connection_pool",                                       :require => false # For Dalli
  gem "config",                           "~>2.2", ">=2.2.3",  :require => false
++<<<<<<< HEAD
 +gem "dalli",                            "=2.7.6",            :require => false
++=======
+ gem "dalli",                            "~>3.0.6",           :require => false
++>>>>>>> d943a50285... Merge pull request #21595 from jrafanie/lock_down_dalli
  gem "default_value_for",                "~>3.3"
  gem "docker-api",                       "~>1.33.6",          :require => false
  gem "elif",                             "=0.1.0",            :require => false

@Fryguy
Copy link
Member

Fryguy commented Dec 7, 2021

@bdunne @jrafanie ^ - it seems we weren't on dalli 3 for morphy, so I don't think there's anything to backport for this. Is that correct?

@bdunne
Copy link
Member

bdunne commented Dec 7, 2021

Yeah, then morphy should be fine without it

@jrafanie
Copy link
Member Author

jrafanie commented Dec 7, 2021

Yeah, it's not worth it, we'd have to bring back

#21265
ManageIQ/manageiq-pods#764
#21280

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

can't cast Rack::Session::SessionId [dashboard/authenticate]
4 participants