fix(remix): Use domains to prevent scope bleed #5570
Conversation
Create a domain and manually push and pop the domain stack to isolate requests. This helps prevent scope bleed issues between transactions.
| @@ -131,4 +131,22 @@ describe('Remix API Loaders', () => { | |||
| }, | |||
| }); | |||
| }); | |||
|
|
|||
| it('makes sure scope does not bleed between requests', async () => { | |||
There was a problem hiding this comment.
Did this test previously fail? IOW, would it actually detect scope bleed if we were to accidentally re-introduce it?
There was a problem hiding this comment.
This test did previously fail - but now it seems to be sometimes passing 🤔, worried it's a flaky test then. @lobsterkatie any ideas for a good test against scope bleed?
There was a problem hiding this comment.
Hmmm... I would assume we basically have to force them to be simultaneous, right? This test would pass regardless, as long as there were only one request/transaction at at time, so maybe the flakiness is coming from the requests just getting through so quickly that they end up being sequential. Could you introduce a variable delay into each request, and set them off at a set interval, to guarantee they'd all be in flight at the same time? I'm picturing something like (assuming 5 requests):
Request 1: Starts at timestamp 0, waits 5 seconds to set its tag, waits another second, finishes
Request 2: Starts at timestamp 1 second, waits 4 seconds to set its tag, waits another second, finishes
...
Request 5: Starts at timestamp 4 seconds, waits 1 second to set its tag, waits another second, finishes
That way, you know that 5 seconds in, all five requests should be trying to set their tags more or less at the same moment, and should also all be finishing at more or less the same time (meaning they'd all be trying to grab scope data to attach to the event roughly simultaneously).
(You might also consider introducing a tiny bit of randomness (wait anywhere between 0.99 and 1.01 seconds to start each request, and the do it again for finishing, for example), just so that the order in which requests set and get their tags is mixed up.)
If everything still comes through cleanly, then I think we'd've proved the point, right?
There was a problem hiding this comment.
Yup, and we've tested on express as well. Thanks Katie, used a simple version of your logic here.
I tried the randomness, but it wasn't working that well, so elected to do this.
Co-authored-by: Onur Temizkan <[email protected]>
Co-authored-by: Onur Temizkan <[email protected]>
size-limit report 📦
|
It's for paramaterized routes: https://remix.run/docs/en/v1/guides/data-loading#route-params
Yes 100% - in retrospect shoulda moved the PR to draft. Was just testing out if different scenarios flake. Will introduce a random element in a bit. |
AbhiPrasad
force-pushed
the
abhi-remix-scope-bleed
branch
from
7664ee9 to
34f9c6d
Compare
|
Ok - this PR kinda ballooned in scope. @lobsterkatie @onurtemizkan mind taking another quick high level review? Once that's done, I'm going to clean up the commits by opening up a new PR and outlining my rationale a little more. I ended up grabbing some nextjs utils, I'll abstract and move those into utils after I get this done. |
|
Looks good to me 👍 |
|
Ok - I'm gonna go ahead and merge this since it unblocks users (and the |
|
Are we not worried that in its final version the remix tests are failing? |
|
@lobsterkatie yeah bad merge - fixed this in #5590! |
getsentry#5570 merged, but the tests were being very flaky. I took another look and figured out that we only needed to wrap our instrumentation with a domain once. Wrapping twice (for both express and built-in) was causing problems. In addition, I moved the wrapping down to the request handling phase, which makes the behaviour more correct.
https://nodejs.org/api/domain.html
Create a domain and manually push and pop the domain stack to isolate requests. This helps prevent scope bleed issues between transactions.