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

sidekiq scope potentially leaks when there's an exception #2012

Closed
sl0thentr0py opened this issue Mar 1, 2023 · 5 comments · Fixed by #2059
Closed

sidekiq scope potentially leaks when there's an exception #2012

sl0thentr0py opened this issue Mar 1, 2023 · 5 comments · Fixed by #2059

Comments

@sl0thentr0py
Copy link
Member

we're manually clearing scope in the middleware but if there's an exception in the job this might not run. We should just use push_scope instead to ensure cleanup.

Try this out with a proper test case and fix it.

@st0012
Copy link
Collaborator

st0012 commented Apr 30, 2023

Yeah you're right, the scope may not be cleared if there's an error. I'll take a look at this.

@st0012 st0012 self-assigned this Apr 30, 2023
@st0012 st0012 added this to the 6.0.0 milestone Apr 30, 2023
@st0012
Copy link
Collaborator

st0012 commented Apr 30, 2023

Ok I should've read my own comment first. I intentionally didn't use ensure there because at that moment, the exception is not reported yet. So we need to NOT clear the scope until we reach ErrorHandler later.

Was there an incident that made you started looking into this?

@sl0thentr0py
Copy link
Member Author

I think @untitaker reported this at some point because he was using the SDK on his mastodon instance

@untitaker
Copy link
Member

So we need to NOT clear the scope until we reach ErrorHandler later.

The problem is that the ErrorHandler also does not clear the scope.

@st0012 st0012 modified the milestones: 6.0.0, 5.10.0 May 26, 2023
@st0012
Copy link
Collaborator

st0012 commented Jun 25, 2023

@untitaker Yes you're right, good catch 👍 I've opened #2059 to address it.

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

Successfully merging a pull request may close this issue.

3 participants