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

Add Channel#closeWithElement #3280

Merged
merged 19 commits into from
Sep 12, 2023

Conversation

tothpeti
Copy link
Contributor

@tothpeti tothpeti commented Aug 22, 2023

This PR adds the requested functionality: #3279

  • Add closeWithElement in Channel
  • Add test case

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

More name ideas: sendThenClose or sendLast.

core/shared/src/main/scala/fs2/concurrent/Channel.scala Outdated Show resolved Hide resolved
@tothpeti
Copy link
Contributor Author

More name ideas: sendThenClose or sendLast.

I think sendAndClose would be pretty clear for the user.

@armanbilge armanbilge changed the title Add Channel#sendClose Add Channel#sendClose Sep 7, 2023
@armanbilge armanbilge changed the title Add Channel#sendClose Add Channel#sendAndClose Sep 7, 2023
@SystemFw
Copy link
Collaborator

I've been trying to figure out the correct semantics for this operation, which I think depends on expected use case, because as it stands, sending is not atomic with closing with this implementation.
In particular cancelling a sendAndClose blocked on a bound means the channel is closed without having the element sent included in the channel's graceful closure.

@SystemFw
Copy link
Collaborator

If I understand the use case correctly, I think the correct behaviour for this operation is to be called closeWithElement, and bypass the bound on a bounded channel without ever blocking, but enqueueing the final element alongside setting closed =true.

@armanbilge
Copy link
Member

Thanks. That sounds right to me, @tothpeti wdyt?

@tothpeti
Copy link
Contributor Author

Thanks. That sounds right to me, @tothpeti wdyt?

Sounds good to me, as well.

@armanbilge armanbilge changed the title Add Channel#sendAndClose Add Channel#closeWithElement Sep 11, 2023
Comment on lines 296 to 297
case obtained @ ((Right(()), Left(Channel.Closed)), List(1)) =>
assertEquals(obtained, expectedSecondCase)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't think this case should ever happen? Is it happening?

Copy link
Contributor Author

@tothpeti tothpeti Sep 11, 2023

Choose a reason for hiding this comment

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

It is happening once or twice during 10000 runs, but it is pretty rare.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how that can happen if everything is working correctly. If the closeWithElement returns that the Channel is already closed, then who is closing it? Seems like a bug 😕

Copy link
Contributor Author

@tothpeti tothpeti Sep 11, 2023

Choose a reason for hiding this comment

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

To be honest, I left this match case there, when we iterated over multiple possible solutions. So, probably this will not happen. I will try it without this match case soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I tried with 100000 runs, and that match case never occurred. So, it can be safely removed. 👍

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for all your work on this!

@armanbilge armanbilge linked an issue Sep 11, 2023 that may be closed by this pull request
@armanbilge armanbilge merged commit 8a4221e into typelevel:main Sep 12, 2023
13 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.

Add Channel#sendClose
4 participants