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 documentation for Channel#send and Channel#close (crystal-lang#8346) #8356

Merged
merged 6 commits into from Nov 26, 2019
Merged

Add documentation for Channel#send and Channel#close (crystal-lang#8346) #8356

merged 6 commits into from Nov 26, 2019

Conversation

lbarasti
Copy link
Contributor

@lbarasti lbarasti commented Oct 20, 2019

Closes #8346

# channel.send(1)
# spawn do
# channel.send(1)
# end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative way to fix the code snippet would be to go buffered

channel = Channel(Int32).new(1)
channel.send(42)
channel.receive # => 42

@@ -182,13 +191,15 @@ class Channel(T)
end

# Receives a value from the channel.
# If there is a value waiting, it is returned immediately. Otherwise, this method blocks until a value is sent to the channel.
# If there is a value waiting, then it is returned immediately. Otherwise, this method blocks until a value is sent to the channel.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added then for consistency with the docs above - both are valid, I believe

src/channel.cr Outdated
# Closes the channel.
# The method prevents any new value from being sent to / received from the channel.
# It wakes up any sender / receiver fibers waiting on the channel.
# It only has effect *after* any previously enqueued value in the channel has been received, up to the channel's capacity.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I appreciate the wording can be improved, so happy to take suggestions here

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably just say

All items successfully sent to the channel can be received before receive considers the channel closed, but send will consider the channel closed before close returns.

src/channel.cr Outdated
# Closes the channel.
# The method prevents any new value from being sent to / received from the channel.
# It wakes up any sender / receiver fibers waiting on the channel.
# It only has effect *after* any previously enqueued value in the channel has been received, up to the channel's capacity.
Copy link
Contributor Author

@lbarasti lbarasti Oct 20, 2019

Choose a reason for hiding this comment

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

I would add that calling close on a closed channel does not have any effect, but such behaviour hasn't been documented in the specs, so I'd better add a test too, if we're keen on being explicit on that

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd document and spec that. You can do that in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done:

  • added to Channel#close docs
Calling `#close` on a closed channel does not have any effect.
  • Added spec
Channel "does not raise nor change its status when it is closed more than once"

src/channel.cr Outdated
@@ -114,6 +114,10 @@ class Channel(T)
end
end

# Closes the channel.
# The method prevents any new value from being sent to / received from the channel.
# It wakes up any sender / receiver fibers waiting on the channel.
Copy link
Contributor

Choose a reason for hiding this comment

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

All fibers blocked in send or receive will recieve Channel::ClosedError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, see

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant to suggest this as a better wording.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will update this later today 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RX14

All fibers blocked in send or receive will receive Channel::ClosedError

is ambiguous in that one might expect to actually receive an object of type ClosedError. I'm opting for

All fibers blocked in send or receive will be awakened with Channel::ClosedError

Let me know what you think

src/channel.cr Outdated
# Closes the channel.
# The method prevents any new value from being sent to / received from the channel.
# It wakes up any sender / receiver fibers waiting on the channel.
# It only has effect *after* any previously enqueued value in the channel has been received, up to the channel's capacity.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably just say

All items successfully sent to the channel can be received before receive considers the channel closed, but send will consider the channel closed before close returns.

src/channel.cr Outdated
# Closes the channel.
# The method prevents any new value from being sent to / received from the channel.
# It wakes up any sender / receiver fibers waiting on the channel.
# It only has effect *after* any previously enqueued value in the channel has been received, up to the channel's capacity.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd document and spec that. You can do that in this PR.

src/channel.cr Outdated Show resolved Hide resolved
Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

Just tiny nitpicks!

spec/std/channel_spec.cr Outdated Show resolved Hide resolved
spec/std/channel_spec.cr Outdated Show resolved Hide resolved
@lbarasti lbarasti requested a review from RX14 November 6, 2019 18:59
@straight-shoota straight-shoota added this to the 0.32.0 milestone Nov 25, 2019
@straight-shoota straight-shoota merged commit 85baa80 into crystal-lang:master Nov 26, 2019
@lbarasti lbarasti deleted the document-channel-send-and-close-behaviour branch November 27, 2019 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Behaviour of unbuffered Channel#send on Channel#close in v0.31.x vs previous versions
3 participants