Fixed channel capacity in NotifyContext function in os/signals package

[erma...@gmail.com]

Hello,

I have an improvement idea for the following line in the os/signals package.

284 c.ch = make(chan os.Signal, 1)

Regarding the underlying function's (Notify) description ("the caller must ensure that channel has sufficient buffer space to keep up with the expected signal rate.") the code can be improved like the following code snippet:

cap := len(signals)

if cap == 0 {

    cap = 1

}

c.ch = make(chan os.Signal, cap)

I wanted to discuss here first before opening my first issue :)

Best regards

[erma...@gmail.com]

Sorry about the broken link, here is the line:

https://github.com/golang/go/blob/912f0750472dd4f674b69ca1616bfaf377af1805/src/os/signal/signal.go#L284

[Sean Liao]

What problem are you trying to solve?

It only makes sense for NotifyContext to receive a single signal since a context can only be cancelled once

[Erman İmer]

I think it is not about the context's cancellation. The signal channel should have a sufficient buffer size.

Please check the former discussion which determined the Notify function's description.

https://github.com/dominikh/go-staticcheck/issues/30

Best regards

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/PpUBZ1hxBbM/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/c1cd6bd9-5307-4a71-9caa-f13c354185ccn%40googlegroups.com.

--

Erman İmer

[Erman İmer]

Another former discussion:

https://github.com/golang/lint/issues/175

--

Erman İmer

[Axel Wagner]

On Mon, Jul 5, 2021 at 12:41 PM Erman İmer <erma...@gmail.com> wrote:

I think it is not about the context's cancellation. The signal channel should have a sufficient buffer size.

1 is a sufficient buffer size. Note that there is *exactly one* read from c.ch in

https://github.com/golang/go/blob/912f0750472dd4f674b69ca1616bfaf377af1805/src/os/signal/signal.go#L287

Therefore, even if the buffer was larger, any extra buffered signals would never be read. So the only possible point to having a larger buffer would be to prevent a writer from blocking on writing to it.

However, as the documentation you are quoting is pointing out, `Notify` (the only writer to that channel) writes the signal non-blockingly. Therefore, there is never any danger of `Notify` getting blocked. The sentence you are quoting (making sure there is enough buffer size) is there to assure you have enough buffer space for a signal to get delivered - not to prevent `Notify` from blocking.

Lastly, even *if* we needed more buffer space, your assumption that we need as much buffer space as there are signals is wrong. A single signal can be caught multiple times. So *if* we needed to be able to handle multiple signals (we don't - again, there is only one read, which is the one cancelling the context), we would also take into account that aspect. And there just is no actually correct size of the buffer, to make sure *all* signals get delivered - the number of signals that can happen before a reader is ready is unbounded.

The code as it is currently is correct.

 

You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAAYkT3MhFw3nmx6Fduv0wRiy10uC1un3whkwYN%2BVNbJPvtApfA%40mail.gmail.com.

[Axel Wagner]

The documentation is actually very specific here:

Package signal will not block sending to c: the caller must ensure that c has sufficient buffer space to keep up with the expected signal rate. For a channel used for notification of just one signal value, a buffer of size 1 is sufficient.

(emphasis mine). This is exactly such a case. We only need one signal value to know the context is cancelled.

[Erman İmer]

Thank you! I was confused about the highlighted part "For a channel used for notification of just one signal value, a buffer of size 1 is sufficient." But I am convinced after realizing the single read on the 289th line. I will also spend some time understanding how Notify writes signals non-blockingly.

Best regards

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAEkBMfGrMgVFTAVzZfcruB94rntR6nqWshOC%2Bg%2BTK6FAeaiogw%40mail.gmail.com.

--

Erman İmer

[Axel Wagner]

FWIW writing to a channel non-blockingly works like this:

select {

case ch <- v:

default:

}

If no communication can proceed (i.e. the write would block) the default case is chosen.

[Erman İmer]

Awesome, thanks! 

--

Erman İmer