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.

Sign up for GitHub

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

Jump to bottom

Make StreamForking resource-safe in the face of cancelation #6610

Merged
merged 8 commits into from
Oct 11, 2022
Merged

Make StreamForking resource-safe in the face of cancelation #6610

merged 8 commits into from
Oct 11, 2022

Conversation

TimWSpence
Copy link
Contributor

Make StreamForking resource safe in the face of cancelation.

StreamForking isn't actually used at the moment but it looks as if the intention is to use it again.

@TimWSpence
Copy link
Contributor Author

The current tests fail because typelevel/fs2#2962 isn't released yet. If anybody has a suggestion of an alternative testing approach then I'm all ears!

@TimWSpence TimWSpence marked this pull request as ready for review September 6, 2022 15:33
@ChristopherDavenport
Copy link
Member

If this is working, can we use this again to get safe shutdowns again?

@TimWSpence
Copy link
Contributor Author

TimWSpence commented Sep 6, 2022 •
edited
Loading

If this is working, can we use this again to get safe shutdowns again?

Sorry, I have no idea 😅 All I saw was this comment which said we need to fix something else. I haven't done the archaeology yet to find out what that is. I just saw this in passing and thought I should fix it as the plan seems to be to use StreamForking again at some point

@armanbilge
Copy link
Member

Ah, I believe that comment is related to this gnarly fs2 issue:

@armanbilge armanbilge mentioned this pull request Sep 6, 2022
Closed
@RafalSumislawski
Copy link
Member

Given that #6656 (fs2 3.2.14) is now merged. Is this PR ready to be merged?

It looks to me like it would make sense to retarget/backport this to 0.23. What do you think?

@TimWSpence
Copy link
Contributor Author

Given that #6656 (fs2 3.2.14) is now merged. Is this PR ready to be merged?

It looks to me like it would make sense to retarget/backport this to 0.23. What do you think?

I'm in favour 😂

@RafalSumislawski RafalSumislawski merged commit 91cffd6 into http4s:main Oct 11, 2022
@RafalSumislawski RafalSumislawski mentioned this pull request Oct 12, 2022
Merged
@armanbilge armanbilge changed the title Make StreamForking resource safe in the face of cancelation Make StreamForking resource-safe in the face of cancelation Oct 12, 2022
@TimWSpence TimWSpence deleted the forking-resource-safety branch November 23, 2022 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristopherDavenport ChristopherDavenport ChristopherDavenport approved these changes

Assignees
No one assigned
Labels
module:ember-server
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TimWSpence @ChristopherDavenport @armanbilge @RafalSumislawski