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

celery: use strongrefs for celery signals #1122

Merged
merged 3 commits into from
Nov 11, 2019
Merged

Conversation

Kyle-Verhoog
Copy link
Member

This hopefully addresses #1011

There's really no reason for us to not use strong references for our signals.

Copy link
Contributor

@jd jd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really dubious on this one. The fact that celery might use weakref should not change anything since the connect functions are not going to be garbage collected anytime. So their binding should not disappear.

At least if this is the case, we should have some sort of test that proves that this fixes anything.

(I don't have a problem with logging more, maybe do a separate PR?)

@Kyle-Verhoog
Copy link
Member Author

I agree that changing the refs doesn't make sense but it did solve the issue for a user.

I attempted to get a regression test by using import gc; gc.collect() at various points in my Django/Celery app but couldn't get a repro. Even if we did get a repro, I'm not sure if it would be deterministic.

It should be fine to make this change (no need for weak refs anyway), we just don't have support for regressions unfortunately 😕.

I'll make a note to revisit once we get the time.

@brettlangdon brettlangdon merged commit 4862c7e into master Nov 11, 2019
@brettlangdon brettlangdon deleted the celery/strongref branch November 11, 2019 21:33
@shuvozula
Copy link

NOTE: Changing the refs did make the Celery traces show up in the UI, when used with ddtrace.contrib.django

@majorgreys majorgreys changed the title [celery] use strongrefs for celery signals celery: use strongrefs for celery signals Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants