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 test case to return zero on a closed channel #8167

Closed
wants to merge 6 commits into from
Closed
Changes from 3 commits
Commits
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
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 ?

std::thread t([&]() {
// Try to write more than buffer size.
size_t out;
for (size_t i = 0; i < 12; ++i) {
ch->Receive(&out);
if (i < buffer_size)
EXPECT_EQ(out, i); // should block after 10 iterations
else
EXPECT_EQ(out, 0U); // after close Channel is called, expected value is 0
}
});

for (size_t i = 0; 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.

t.join();
delete ch;
}

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