WIP: Maintain thread context in spawned threads for nested sideloads #496
Conversation
…ontext when the execution context changes loses context for nested sideloads that are spawned by other sideloads
|
I think that may cause memory leaks as any data added in a thread won’t be cleaned down at the end of the thread. I can’t think why removing those lines fixes the issue you’re seeing as the context should be set back up at the start of a new thread (execution context). |
|
@MattFenelon It only seems to happen when the includes are nested and it looked like when I was debugging that in my example of my request with a handful of sideloads plus |
|
I wonder if it’s because the execution context hasn’t changed, causing the if statement body to not execute, which would copy the context to Thread.current again. In other words,
|
|
Hmmm, this sounds like a possibility. Any idea how to handle that? |
|
I think there'd need to be two changes. The first is to always set the thread variables at the start of the work and remove the The second thing is to fix the clearing of the thread/fiber storage, before more work is scheduled on that thread, while removing What do you think? |
|
@MattFenelon Yeah that sounds like a good plan |
Thread and fiber locals are being cleared on completion of every future. If the promise ran on the same thread as the request, or the same thread as the last promise, the locals are then not available. The execution context hasn't changed in those circumstances so the code that sets the locals at the start of a promise doesn't run. 1. Thread 1 runs 2. Sets context 3. Loads scope 4. Clears context 5. Thread 1 runs next scope 6. Context hasn’t changed so doesn’t set it again 7. Loads scope 8. Fails because the code relying on the context causes an exception because the locals aren't available. Instead always set the thread and fiber locals and only clear those that we know are new. This should avoid the lost locals and also avoid memory leaks between promises. See: graphiti-api#496
…497) * Fix lost thread and fiber locals Thread and fiber locals are being cleared on completion of every future. If the promise ran on the same thread as the request, or the same thread as the last promise, the locals are then not available. The execution context hasn't changed in those circumstances so the code that sets the locals at the start of a promise doesn't run. 1. Thread 1 runs 2. Sets context 3. Loads scope 4. Clears context 5. Thread 1 runs next scope 6. Context hasn’t changed so doesn’t set it again 7. Loads scope 8. Fails because the code relying on the context causes an exception because the locals aren't available. Instead always set the thread and fiber locals and only clear those that we know are new. This should avoid the lost locals and also avoid memory leaks between promises. See: #496 * Fix account for Fiber.current.storage not being available * Fix account for Fiber.current.storage not being available 2/
|
Closed in favor of #497 |
When there are multi-level sideloads, the thread context is not maintained when sideloads spawn sideloads, i.e. when a sideload of "permissions.user" is requested. In practice this makes the context object available in resource methods (used to access controller methods) unavailable.
In my app this was causing intermittent 500 errors when it couldn't access the controller to grab the pundit context.
@MattFenelon Removing these lines fixes the issue, but I'm not sure if this will cause other unintended problems?