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

Proposal to remove UnbufferedChannel #24831

Closed
tarekgh opened this issue Jan 29, 2018 · 9 comments
Closed

Proposal to remove UnbufferedChannel #24831

tarekgh opened this issue Jan 29, 2018 · 9 comments

Comments

@tarekgh
Copy link
Member

tarekgh commented Jan 29, 2018

UnbufferedChannel is a special channel that has to be used carefully and if not, we can run into some problems. here is some example demonstrates the problem:

reading code:

                    while (true)
                    {
                        if (!reader.TryRead(out int dequeuedNumber))
                        {
                            if (!reader.WaitToReadAsync().GetAwaiter().GetResult())
                            {
                                break;
                            }

                            try
                            {
                                dequeuedNumber = reader.ReadAsync().GetAwaiter().GetResult();
                            }
                            catch (ChannelClosedException)
                            {
                                break;
                            }
                        }
                    }

Writing code:

                    while (true)
                    {
                        if (!writer.TryWrite(incrementedValue))
                        {
                            if (writer.WaitToWriteAsync().GetAwaiter().GetResult())
                            {
                                writer.WriteAsync(incrementedValue).GetAwaiter().GetResult();
                            }
                        }
                    }

That will cause the UnbufferChannel to hang because TryRead can succeed against a WriteAsync, but it can't against a WaitToWriteAsync because there wouldn't be anything to read. And conversely for TryWrite and WaitToReadAsync.

The proposal here is to remove the UnbufferedChannel from 2.1 as looks nobody using it and we can add later if needed.

@tarekgh
Copy link
Member Author

tarekgh commented Jan 29, 2018

@stephentoub please add aspnet guys who can advise regarding that.

@stephentoub
Copy link
Member

stephentoub commented Jan 29, 2018

FWIW, I think that example is a bit complicated. Maybe a simplified view of the issue is this...

The nature of an unbuffered channel is that, well, there isn't a buffer. That means the reader and writer need to be at the channel at the exact same time, data or slot in hand, in order to coordinate. In the case of Write/ReadAsync, that's easy: one shows up, and if the other is already there, it gives/takes, otherwise it registers with the channel and is signaled later when its counterpart shows up. That counterpart can be either the XxAsync operation or the TryXx operation. The problem happens when neither side uses the XxAsync variant. In that case, both can't be "at" the channel at the same time, since they're each effectively locking access with mutual exclusion. So a TryWrite when there isn't currently a ReadAsync will always fail, and conversely a TryRead when there isn't currently a WriteAsync will always fail.

To put this in code, if you have:

// Producer
foreach (var item in items)
{
    await c.WriteAsync(item);
}

// Consumer
while (await c.WaitToReadAsync())
{
    while (c.TryRead(out item)) { ... }
}

that'll work fine. The writer will come along and register that data is available; that'll cause the WaitToReadAsync to wake up and the TryRead to read that same data. But now if you change it to:

// Producer
foreach (var item in items)
{
    while (!c.TryWrite(item));
}

// Consumer
while (await c.WaitToReadAsync())
{
    while (c.TryRead(out item)) { ... }
}

this will always fail with the unbuffered channel, as it's not possible for the reader and the writer to be there at the same time (the TryXx method is a moment-in-time operation... it doesn't wait at all for someone to show up). With an unbuffered channel, at least one side must interact with the channel using the XxAsync method, or no data will be transferred.

The unbuffered channel is just an oddity in this manner, because there isn't a buffer. Neither the unbounded or the bounded channel suffers from a similar issue.

@stephentoub
Copy link
Member

@anurse, does ASP.NET use Channel.CreateUnbuffered at all? We're thinking we may want to ditch it for the initial release and only add it back if there's actually demand for it. Or, anyone else have a strong need for it?

@analogrelay
Copy link
Contributor

No, we don't use unbuffered at all. We use unbounded (but shouldn't) and bounded only.

@stephentoub
Copy link
Member

Thanks, @anurse. @tarekgh, let's go ahead and remove it for now. We can add it back in the future if folks share a real need for it (rather than my just having added it because it's a concept that exists in related frameworks).

@tarekgh
Copy link
Member Author

tarekgh commented Jan 30, 2018

@stephentoub I'll remove it. do you prefer keeping the source and test files around for this channel or we just delete it and we can get it from git history if we needed it in the future?

@stephentoub
Copy link
Member

Thanks. I'd prefer we just delete it. Code like that has a very good chance of rotting, and we can always get it from history if needed.

@stephentoub
Copy link
Member

Addressed by dotnet/corefx#26689

@tarekgh tarekgh reopened this Jan 31, 2018
@tarekgh
Copy link
Member Author

tarekgh commented Jan 31, 2018

I'll need to use this issue for porting the code to the preview branch.

@tarekgh tarekgh closed this as completed Jan 31, 2018
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants