Skip to content

fix: Ensure progress callback live long enough to be used#205

Merged
tmathern merged 7 commits intomainfrom
mathern/dangling-callback-take-2
Apr 9, 2026
Merged

fix: Ensure progress callback live long enough to be used#205
tmathern merged 7 commits intomainfrom
mathern/dangling-callback-take-2

Conversation

@tmathern
Copy link
Copy Markdown
Collaborator

@tmathern tmathern commented Apr 9, 2026

As in title!

Fixes #199

@tmathern tmathern marked this pull request as ready for review April 9, 2026 16:39
@tmathern tmathern requested a review from gpeacock April 9, 2026 16:39
@tmathern tmathern self-assigned this Apr 9, 2026
@tmathern tmathern requested a review from ok-nick April 9, 2026 18:00
Copy link
Copy Markdown
Member

@gpeacock gpeacock left a comment

Choose a reason for hiding this comment

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

This looks good. My only concern is if someone can still get into trouble using the old api where we know there is a crash bug. Maybe we can deprecate them or put in something to prevent that case. But this looks fine for providing a case that works.

@tmathern
Copy link
Copy Markdown
Collaborator Author

tmathern commented Apr 9, 2026

This looks good. My only concern is if someone can still get into trouble using the old api where we know there is a crash bug. Maybe we can deprecate them or put in something to prevent that case. But this looks fine for providing a case that works.

Let's do both: Deprecate, and explain in the deprecation why we deprecate.
(I like giving people some time to migrate, especially those who may not use the progress callbacks and hence won't hit the bug)

@tmathern tmathern merged commit 0322d67 into main Apr 9, 2026
9 checks passed
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.

[Bug report] Progress callback function gets destroyed

2 participants