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

Add test case to return zero on a closed channel #8167

Closed
wants to merge 6 commits into from
Closed

Add test case to return zero on a closed channel #8167

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
10d0306
Add test case to return zero on a closed channel
Feb 5, 2018
1891245
Rename method
Feb 5, 2018
e611309
Fix test
Feb 5, 2018
46acca0
Move receiving in same thread
Feb 6, 2018
1861b36
Fixed based on review comments
Feb 6, 2018
e06ee27
review comments
Feb 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions paddle/framework/channel_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,32 @@ TEST(Channel, SufficientBufferSizeDoesntBlock) {
delete ch;
}

// This test tests that CloseChannel returns a value of zero
// immediately to all receivers that are trying to receive from the channel.
TEST(Channel, ReceiverGetsZeroOnClosedChannel) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the purpose of this PR is to add a unit test to ensure that

  1. receiving from a closed buffered channel returns residual values and then zeros values, and
  2. receiving from a closed unbuffered channel doesn't block and returns zero.

then this PR looks too simple to ensure the above two invariants.

Copy link
Author

Choose a reason for hiding this comment

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

I think one of the axioms was that : A receive from a closed channel returns the residual values and then zeros.
This PR tries to validate that #1 you explained above holds since we aren't explicitly checking for the zero value anywhere else.

const size_t buffer_size = 10;
auto ch = MakeChannel<size_t>(buffer_size);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This invariant should be verified with buffered and unbuffered channels.

Copy link
Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Author

Choose a reason for hiding this comment

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

On a side note, correct me if I am wrong @wangkuiyi but for an unbuffered channel, since the buffer size is zero, there would be no residual values. So trying to read from a closed channel should just return a zero value, right ?


for (size_t i = 1; i <= buffer_size; ++i) {
EXPECT_EQ(ch->Send(&i), true); // sending should not block
}

CloseChannel(ch);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe receive some values before CloseChannel and checks that receivings after CloseChannel return the residual values.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for pointing that out! Will fix it in a commit.


// Now try receiving for more number of times than buffer size
// after channel is closed
int out;
for (size_t i = 1; i <= 12; ++i) {
ch->Receive(&out);
if (i <= buffer_size)
EXPECT_EQ(out, i); // same value as was written by senders
else
EXPECT_EQ(out, 0U); // 0 after all elements are emptied from a closed channel
}

delete ch;
}

TEST(Channel, ConcurrentSendNonConcurrentReceiveWithSufficientBufferSize) {
const size_t buffer_size = 10;
auto ch = MakeChannel<size_t>(buffer_size);
Expand Down