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

Global namespacing that applies to all threads #158

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeremy
Copy link
Member

@jeremy jeremy commented Dec 19, 2024

Namespacing is thread-local currently, so it's easy to lose, e.g. when running parallel tests, Capybara tests, or executor pools.

Changing that is a major compatibility break, so we introduce a non-thread-local Kredis.global_namespace attribute that behaves as we'd expect.

When Kredis.namespace is set, it's appended to the global namespace.

Namespacing is thread-local currently, so it's easy to lose, e.g.
when running parallel tests, Capybara tests, or executor pools.

Changing that is a major compatibility break, so we introduce a
non-thread-local Kredis.global_namespace attribute that behaves
as we'd expect.

When Kredis.namespace is set, it's appended to the global namespace.
@maxim
Copy link

maxim commented Jan 21, 2025

@jeremy FWIW, our parallelized test suite started failing randomly after upgrading to Rails 8 due to this issue. The solution in #156 didn't fix it for us (still seeing random failures). However, this global-namespace branch seems to completely eliminate the failures. Hoping to see it released soon.

@ibrahima
Copy link

ibrahima commented Jan 22, 2025

We also have been experiencing flaky tests recently, and it seems to be tests that depend on Kredis functionality. We also recently upgraded to Rails 8 so it could be the same issue. We do have the patch from #152 in our codebase so it seems like that's no longer enough to ensure that the namespace is set properly. It would be great to have this fix available in a released version so that our test suite can be more reliable. No pressure of course, we can try to work around it for now. I'll try out this branch to see if it helps. Thanks!

Edit: It seems like (at least my machine) this patch makes a different test that uses Kredis more flaky, so I'm not sure if this solves the issue. Actually, I'm not sure I understand the fix - if the issue is that the thread-local namespace is lost, how does appending a global namespace help there? Wouldn't that still result in multiple threads sharing the same namespace and conflicts? It seems like it would solve the problem of clearing the entire DB when we shouldn't, but two test threads could still share the global namespace where one thread could clear the whole namespace for the other thread. I guess because I don't understand how the thread local namespace is getting lost, I don't understand when this additional fix could still go awry.

Edit2: Ah, perhaps the monkey patch is interfering with this fix. I think without it, the other test is not flaking so far. And now I understand the fix better - because the default parallelization option in Rails is to use processes, not threads, setting the global attribute on the Kredis namespace sets it for all threads in the process, and ensures that it's not lost if some code is executing in a different thread that happens to not have the thread local variable set. If you do run parallel tests in multiple threads, you might have different experiences, but I don't know if this PR makes things any worse for that scenario. It kind of feels like in that scenario different threads might clobber each others' global namespaces, but I don't know for sure.

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

Successfully merging this pull request may close these issues.

3 participants