Bug in select when one channel closed

[kostya]

this example work fine in 0.31.1 and failed in 0.32.1:

ch1 = Channel(Int32).new
ch2 = Channel(Int32).new

spawn do
  sleep 0.1
  ch1.send(1)
  ch1.close
end

spawn do
  sleep 0.2
  ch2.send(2)
  ch2.close
end

2.times do
  select
  when v = ch1.receive
    puts "ch1 - #{v}"
  when v = ch2.receive
    puts "ch2 - #{v}"
  end
end

ch1 - 1
Unhandled exception: Channel is closed (Channel::ClosedError)
  from /Users/kostya/Downloads/crystal-0.32.1-1/src/channel.cr:542:7 in 'default_result'
  from /Users/kostya/Downloads/crystal-0.32.1-1/src/channel.cr:269:9 in 'select_impl'
  from /Users/kostya/Downloads/crystal-0.32.1-1/src/channel.cr:390:12 in 'select'
  from 2.cr:17:3 in '__crystal_main'
  from /Users/kostya/Downloads/crystal-0.32.1-1/src/crystal/main.cr:97:5 in 'main_user_code'
  from /Users/kostya/Downloads/crystal-0.32.1-1/src/crystal/main.cr:86:7 in 'main'
  from /Users/kostya/Downloads/crystal-0.32.1-1/src/crystal/main.cr:106:3 in 'main'

after reading this https://gist.github.com/bcardiff/289953a80eb3a0512a2a2f8c8dfeb1db, i tried receive?, but it just return nil forever. I think this error should only raise when all channels closed.

[RX14]

I don't like that there's not a way to do this? Is go really the same?

[bcardiff]

The equivalent to go's is to use receive? that would return nil on close. But that will not play nice for nillable message types.

[RX14]

@bcardiff but that would return nil immediately. Is there no way to ignore closed channels in select statements?

i tried receive?, but it just return nil forever

[bcardiff]

Right. Using receive? is not enough.

While working on #8304 I recall that we thought/decide that executing a select over an already closed channel was a bit of a smell. If it is already closed you should not do a select. If it happens to become closed while waiting it is useful to know about that event. But that left out the chance to dynamically build the list of channels to listen (unless the non-public Channel API is used).

Together with the above, we wanted to unify some behaviors.

We have

ch = Channel(Int32).new
ch.close
ch.receive  # raise: Channel is closed (Channel::ClosedError)

and

ch.receive? # => nil

So on blocking and non-blocking select over a closed channel, we have something similar

select
when a = ch.receive
  a
end # raise: Channel is closed (Channel::ClosedError)

select
when a = ch.receive?
  a
end # => nil

Maybe the behavior of ch.receive? on select should be equivalent to return nil only when the channel is closed while waiting so to signal de closing action. But is odd that outside the select that will return immediately.

I liked that select; when S; end is the same as S, which is the current semantics in 0.32.

[kostya]

Usage of select is sync state while receive data from multiple channels, its normal that some of them can be already closed. I think it should raise error only when all channels closed.

[RX14]

Perhaps receive_select_action(allow_closed: true)?

[bcardiff]

@kostya you mean to raise if all the channels are closed from the beginning and the select is blocking. Plus trigger the when branch when the channel is closed while waiting?

What do you suggest for non-blocking select? trigger the else if all are closed or raise (even if receive? is used)

(maybe @firejox will want to join the conversation).

@RX14 let's focus on semantics first. But I would like to avoid parameters within the select/when

[bcardiff]

Comparing things with Go. The following code executes the c1 or c2 branch without guarantee. So an already closed channel triggers the blocking select also in Go.

package main

import (
	"fmt"
	"time"
)

func main() {
	c1 := make(chan int)
	c2 := make(chan int)

	close(c1)
	go func ()  {
		c2 <- 2
	}()
	time.Sleep(500 * time.Millisecond) // ensure c2 has a message

	go func() {
		fmt.Println("a")

		select {
		case m, x := <-c1:
			fmt.Println("c1 ", m, x)
		case m, x := <-c2:
			fmt.Println("c2 ", m, x)
		}

		fmt.Println("z")
	}()

	time.Sleep(500 * time.Millisecond) // wait for the select
	fmt.Println("done")
}

[RX14]

@bcardiff the semantics for what i proposed would be that you can specify a parameter which means recieve wouldn't raise when that channel is closed when used in select, unless all channels were closed.

[ysbaddaden]

Here are the relevant tests for Go's select over closed channels:
https://github.com/golang/go/blob/master/test/chan/select3.go#L194-L216

Selects with closed channels behave like ordinary operations: receive doesn't block while send panics; Channels thus behave the same whatever if they're running inside a select or not.

Yet, if multiple channels are ready (before blocking), select will choose a channel to read/write from/to at random, which may allow to read from channel A while channel B is closed if running inside a loop (with a sleep(0) to avoid burning/blocking the current thread)?

[ysbaddaden]

I agree with @bcardiff: there shouldn't be any parameter to select.

[bcardiff]

I was going to propose to allow nil to receive and send on select as a way to turn off channels subscriptions.

It sounded weird but... https://dave.cheney.net/2013/04/30/curious-channels (Jump to "A nil channel always blocks").

Ideas?

[bcardiff]

Alternatives:

1. Create a NilChannel or similar to avoid polluting Nil
2. Just create a new channel in the loop and never send messages.

ch1 = Channel(Int32).new
ch2 = Channel(Int32).new

spawn do
  sleep 0.1
  ch1.send(1)
  ch1.close
end

spawn do
  4.times do
    sleep 0.2
    ch2.send(2)
  end
  ch2.close
end

nil_channel = Channel(Int32).new

while ch1 != nil_channel || ch2 != nil_channel
  select
  when v = ch1.receive?
    if v
      puts "ch1 - #{v}"
    else
      ch1 = nil_channel
      puts "ch1 closed"
    end
  when v = ch2.receive?
    if v
      puts "ch2 - #{v}"
    else
      ch2 = nil_channel
      puts "ch2 closed"
    end
  end
end

[firejox]

I think the ignoring behavior is a bug in 0.31.1. Closing channel during select is not well implemented in 0.31.1, so the select ignores the closed channel. If we change the length of sleeping time and the times of the select. The program will raise the exception as #8231 or hang forever.

Besides, I think the new feature is unneeded. We can just maintain available channels on each select until there is no available channel.

[kostya]

Ok. Looks like need to rethink how to use 'close' on channels. And actually not use it in my cases. I used it like in sockets, just close when it not needed (in sockets to free descriptors). If i remove close from example it works as expected. So if i would create millions of short-living channels (for timeouts for example), and not closing them, and its ok for GC, so i guess i not going to use 'close'.

[bcardiff]

We are all learning and trying to get better here :-)

We can create a NilChannel that can be probably even a struct, not inheriting Channel, and implement the _select_action methods for it to work on select. That would be slightly better for the GC.

But also you can have a single (per each type) of the current channels also.