-
|
Hi Sentry team, We’ve been testing the new feature flag functionality (currently in beta) and ran into an issue that appears to have been addressed by #4034 The fix introduces a lock to prevent concurrency issues, but we were wondering why Thanks in advance for your help! |
Beta Was this translation helpful? Give feedback.
Replies: 1 comment · 3 replies
-
|
Thanks for reaching out!
The first order question being "why lock"? It solved a problem in a way I knew how to solve it. There's no deeper meaning to it than that. If this has caused deadlocks for you or performance degradation please open an issue and document the problem as best you can. We'll address as quickly as possible. The original question being "Why share FlagBuffer across contexts". I don't think it should be shared but sharing it is a choice (or mistake) people make. In my opinion, the Scope should be forked when you enter a new thread or async context but that assumes something about the way your program works. "Why not make this threadlocal". Two reasons, 1) we need it to work in async code and 2) we need the state of the parent to be copied to the child. threadlocals don't solve for either of those conditions. Phrasing the question differently "why not use a contextvar". That gives us the state of the parent in the child and it works in async code. But now we run into a new set of problems. Mutable data-structures which are mutated in the child are also mutated in the parent and in other children. To solve this we need to do something. These are the ideas I came up with to solve this problem:
There may be more options but this is what I could think of. Copying the contextvar (flag buffer) on Having said all that. Is it valuable to scope the FlagBuffer to a thread or async context? I can write code like this in python. x = 1
t = threading.Thread(target=lambda: global x; x = x + 1)
t.start()
t.join()
if x == 2:
raise Exception("oops")
At the moment I'm erring on the side of leaving the lock in. That being said, I only know what I know. If I've made a mistake or I've missed something or if you know a different way please correct me and I can update the code. Thanks again for reaching out! |
Beta Was this translation helpful? Give feedback.
-
|
Thank you very much for taking the time to write such a detailed answer. To give more context on our case, we observed issues in two scenarios:
import asyncio
from sentry_sdk import configure_scope
from contextlib import contextmanager
from concurrent.futures import ThreadPoolExecutor
class CustomJob:
def __init__(
self,
max_threads: int = 10,
pool_size: int = 1000,
):
self.__max_threads = max_threads
self.__pool_size = pool_size
self.__event_loop = asyncio.new_event_loop()
self.__executor = ThreadPoolExecutor(max_workers=self.__max_threads)
@contextmanager
def _sentry_metadata(self):
with configure_scope() as scope:
scope.set_tag('job.name', self.observability_name)
yield
def perform(
self, *args, **kwargs
):
with self._sentry_metadata():
self.__event_loop.run_until_complete(self.__do_async_stuff())
self.__event_loop.close()
async def __do_async_stuff(self, ids: list[int]):
# ...
passI suspect that we might not be handling Sentry’s scope correctly in this job. From my understanding, we should be forking the scope with new_scope() in each async task to ensure proper isolation. Could you confirm if this is the correct approach? Again, thank you for your time. We’ll test the feature with the latest version and monitor its behavior. |
Beta Was this translation helpful? Give feedback.
-
|
@antoniobg Sorry for the delay! Yes, that's the correct implementation. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks, @cmanallen! By the way, we've given it another try with the newer SDK version and haven't seen the issue anymore (also no deadlocks or performance degradation). |
Beta Was this translation helpful? Give feedback.
Thanks for reaching out!
The first order question being "why lock"? It solved a problem in a way I knew how to solve it. There's no deeper meaning to it than that. If this has caused deadlocks for you or performance degradation please open an issue and document the problem as best you can. We'll address as quickly as possible.
The original question being "Why share FlagBuffer across contexts". I don't think it should be shared but sharing it is a choice (or mistake) people make. In my opinion, the Scope should be…