-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Receive from closed channel #8175
Receive from closed channel #8175
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I just pushed something similar in my latest commit after your comment. Let's merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@kavyasrinet I am afraid that I will be at the meeting for the whole night, so might not have time to polish this PR, which was originally created only for your reference. Please feel free to commit to my branch of this PR to fix the CI problems. Thanks! |
Sure, will do. |
c67c424
paddle/framework/channel_test.cc
Outdated
EXPECT_EQ(ch->Send(&i), true); // sending should not block | ||
} | ||
|
||
int out; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int out
==> size_t out
paddle/framework/channel_test.cc
Outdated
@@ -119,6 +150,7 @@ TEST(Channel, BufferedChannelCloseUnblocksReceiversTest) { | |||
int data; | |||
// All reads should return false | |||
EXPECT_EQ(ch->Receive(&data), false); | |||
EXPECT_EQ(data, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think the data
should be zero when the channel was closed. If the element of channel is Tensor*
and the channel has been closed, what will the data
be after ch->Receive(&data)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be the zero value of that return type, as mentioned here: https://dave.cheney.net/2014/03/19/channel-axioms
Here are the zero values: https://golang.org/ref/spec#The_zero_value
For now I have removed this in my current commit, but once we decide we can introduce that behavior again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is that C++ syntax doesn't define zero value of all types like Go does. So, basically, we have no idea how to write a C++ code snippet to clear the value of any data type to zero.
|
||
for (size_t i = 0; i < buffer_size; ++i) { | ||
EXPECT_EQ(ch->Receive(&out), | ||
false); // after receiving residual values, return zeros. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This annotation should be removed or changed.
// after receiving residual values, return zeros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.