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

Rethink our strategies towards handling channels #192

Open
yuxincs opened this issue Feb 1, 2024 · 0 comments
Open

Rethink our strategies towards handling channels #192

yuxincs opened this issue Feb 1, 2024 · 0 comments
Labels
channels false positive Requires more analysis and support

Comments

@yuxincs
Copy link
Contributor

yuxincs commented Feb 1, 2024

Currently in NilAway we forbid sending to and receiving from nilable channels by creating corresponding consumers for them. However, in practice this will not result in panics.

See the following behaviors regarding nil / closed channels (taken from [this SO post], slightly modified (https://stackoverflow.com/questions/43616434/closed-channel-vs-nil-channel)):

  1. A send to a nil channel blocks forever
  2. A receive from a nil channel blocks forever
  3. A send to a closed channel panics
  4. A receive from a closed channel returns the zero value immediately

Only case 3 (sending to a closed channel) will result in a panic. The other cases arguably may not be within scope of NilAway's analysis. Moreover, NilAway currently only tracks nilabilities of channels, but not the states of them, making it unable to really handle case 2 and 4.

We will definitely need to rethink our strategies towards handling channels since the current approach raises a lot of false positives. For example, below is a perfectly valid and very popular pattern in Go:

func foo(ctx context.Context) {
  // ...
  select {
    case ctx.Done():
      return
    default:
  }
  // ...
}

which checks if the context is done, and if not continue to handling next rounds of business. Note that here ctx.Done() return a nilable channel, but it is perfectly fine to read from it (without panics, and in this case, even without blocking forever due to the select statement).

See also issue #98 for more discussions.

@yuxincs yuxincs added channels false positive Requires more analysis and support labels Feb 1, 2024
yuxincs added a commit that referenced this issue Feb 2, 2024
See issue #192 for a more detailed discussion on handling channels in
NilAway. To make this PR more self-contained, below is a partial copy of
the discussion there:

See the following behaviors regarding nil / closed channels (taken from
[this SO
post](https://stackoverflow.com/questions/43616434/closed-channel-vs-nil-channel),
slightly modified):
> 1. A send to a nil channel blocks forever
> 2. A receive from a nil channel blocks forever
> 3. A send to a closed channel panics
> 4. A receive from a closed channel returns the zero value immediately

Only case 3 (sending to a closed channel) will result in a panic. The
other cases arguably may not be within scope of NilAway's analysis.
Moreover, NilAway currently only tracks nilabilities of channels, but
not the states of them, making it unable to really handle case 2 and 4.

Since this introduces a lot more FPs than TPs (if any), this PR removes
logic to require sending to/receiving from nonnil channels. We leave
properly handling channels as future work (especially around tracking
states of channels).

The test cases have been updated to reflect this. Specifically, we kept
the test cases there to now serve as negative cases in case for future
development of such a support.

Fixes #98 
Fixes #167

---------

Co-authored-by: Sonal Mahajan <101232472+sonalmahajan15@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
channels false positive Requires more analysis and support
Projects
None yet
Development

No branches or pull requests

1 participant