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

Release 1.7.0 -- no longer use Fiber - reverts Fiber changes in release 1.6.0 #99

Merged
merged 1 commit into from
May 1, 2024

Conversation

professor
Copy link
Contributor

@professor professor commented Apr 30, 2024

Fixes #96 and Fixes #98

Unless a Fiber is initialized with Fiber.new(storage: nil) then there is cross Fiber pollution of the RequestStore if the RequestStore is initialized or used in any parent Fiber.

Notes

1.6.0 added a method RequestStore.scope -- I was tempted to keep the method in 1.7.0 but decided to remove it for two reasons even though it is a breaking change.

  • We want 1.7.0 to have the same functionality as 1.5.x
  • Whenever we want to use Fiber in a 2.x.x, I suspect we will want a different API than RequestStore.scope

@professor professor changed the title Reverts release 1.6.0 - no longer use Fiber Release 1.7.0 -- no longe use Fiber - reverts Fiber changes in release 1.6.0 Apr 30, 2024
@professor professor changed the title Release 1.7.0 -- no longe use Fiber - reverts Fiber changes in release 1.6.0 Release 1.7.0 -- no longer use Fiber - reverts Fiber changes in release 1.6.0 Apr 30, 2024
professor added a commit to professor/paper_trail that referenced this pull request Apr 30, 2024
The 1.6.0 release is getting reverted.
See steveklabnik/request_store#99
@smudge
Copy link

smudge commented May 1, 2024

This revert would work for me & my team, thank you. 👍

My only thought would be to add a regression test, something like the one in #97. I can confirm that (on Ruby 3.2+) this fails for me on 1.6 and passes with the changes in this PR:

  def test_concurrent_stores
    Thread.new { RequestStore.store[:foo] = 'bar' }.join
    value = Thread.new { RequestStore.store[:foo] }.value

    assert value != 'bar', 'Thread 2 should not see the value set by Thread 1'
  end

@professor
Copy link
Contributor Author

Thanks. I'm hoping to add a regression test as a separate PR.

@steveklabnik steveklabnik merged commit ded0d62 into steveklabnik:master May 1, 2024
10 checks passed
@steveklabnik
Copy link
Owner

Thank you so much, both of you. <3

@steveklabnik steveklabnik mentioned this pull request May 1, 2024
@steveklabnik
Copy link
Owner

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