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

Fix lifetime issue with Deferred. #2534

Merged
merged 1 commit into from Oct 28, 2023
Merged

Fix lifetime issue with Deferred. #2534

merged 1 commit into from Oct 28, 2023

Conversation

danielt1263
Copy link
Collaborator

This is in response to PR #2533. It includes the test that was added in that PR along with the fix.

…subscriptions to the latter keep its factory closure alive #2533
@freak4pc
Copy link
Member

Thanks both @danielt1263 & @nikolaykasyanov
The fix looks good but I'm quite worried about changing the behavior (even if it's wrong) with a patch / minor release. Not sure it's a huge issue but let me think about it a bit for a day or two :)

If you have large code-bases it might be worth running it to confirm it works as expected at those scales.

@danielt1263
Copy link
Collaborator Author

danielt1263 commented Jul 13, 2023

I have some largish code bases, but I almost never use deferred in my RxSwift code. (I use it all the time in Combine, but not Rx). So they wouldn't be good tests.

@nikolaykasyanov
Copy link

I did it a bit of smoke testing and manual review (fairly big code base, around 20 instances of .deferred only though, if we don't count RxSwift itself), the change looks good from our side.

@freak4pc freak4pc merged commit 214f7ff into ReactiveX:main Oct 28, 2023
5 of 12 checks passed
@danielt1263 danielt1263 deleted the Update-deferred-lifetime-to-match-create branch October 28, 2023 20:31
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

3 participants