slack-go / slack

Slack API in Go, originally by @nlopes; Maintainers needed, contact @parsley42
https://pkg.go.dev/github.com/slack-go/slack
BSD 2-Clause "Simplified" License
4.64k stars 1.13k forks source link

Socketmode panic, write on closed channel #1125

Closed krzysztofjopekacaisoft closed 1 year ago

krzysztofjopekacaisoft commented 1 year ago

What happened

panic: send on closed channel

goroutine 47 [running]:
github.com/slack-go/slack/socketmode.(*Client).receiveMessagesInto(0x14000243950, {0x101c6e948, 0x1400043cac0}, 0x14000676c60, 0x1400061a6c0)
        /Users/censored/go/pkg/mod/github.com/slack-go/slack@v0.11.3/socketmode/socket_mode_managed_conn.go:484 +0x64c
github.com/slack-go/slack/socketmode.(*Client).runMessageReceiver(0x14000243950, {0x101c6e948, 0x1400043cac0}, 0x14000676c60, 0x1400061a6c0)
        /Users/censored/go/pkg/mod/github.com/slack-go/slack@v0.11.3/socketmode/socket_mode_managed_conn.go:363 +0x50
github.com/slack-go/slack/socketmode.(*Client).run.func4()
        /Users/censored/go/pkg/mod/github.com/slack-go/slack@v0.11.3/socketmode/socket_mode_managed_conn.go:133 +0x94
created by github.com/slack-go/slack/socketmode.(*Client).run
        /Users/censored/go/pkg/mod/github.com/slack-go/slack@v0.11.3/socketmode/socket_mode_managed_conn.go:128 +0x948
Exiting.

Looks like channel got closed: https://github.com/slack-go/slack/blob/6f5eda2a6c1e50c8430bb07657bbe8be0def2b37/socketmode/socket_mode_managed_conn.go#L58 before message got passed to the channel: https://github.com/slack-go/slack/blob/6f5eda2a6c1e50c8430bb07657bbe8be0def2b37/socketmode/socket_mode_managed_conn.go#L485 because we don't wait for completion: https://github.com/slack-go/slack/blob/6f5eda2a6c1e50c8430bb07657bbe8be0def2b37/socketmode/socket_mode_managed_conn.go#L126

Expected behavior

No panic

Steps to reproduce

Start socketmode slack client

reproducible code

https://github.com/slack-go/slack/blob/master/examples/socketmode/socketmode.go

Versions

tiantangkuohai commented 1 year ago

I encounter the same problem đŸ˜”

krzysztofjopekacaisoft commented 1 year ago

@tiantangkuohai I think downgrade to 0.11.2 prevents the issue.

kanata2 commented 1 year ago

Thanks for reporting. I'll confirm as soon as possible.

kanata2 commented 1 year ago

Related: https://github.com/slack-go/slack/pull/1094

kanata2 commented 1 year ago

I understand the problem. I'd like to think a bit about a solution. If the problem is serious, please use v0.11.2.

tiantangkuohai commented 1 year ago

Thanks @krzysztofjopekacaisoft @kanata2 . I use v0.11.2, and it works for me. I found that client needs to reconnect to Slack server continually from logs. But debug_info of Slack WebSocket message says approximate_connection_time is 18060 seconds. "debug_info": { "host": "applink-6", "build_number": 20, "approximate_connection_time": 18060 },

Reconnect logs: socketmode: 2022/11/09 20:46:44 socket_mode_managed_conn.go:167: Reconnecting due to websocket: close 1006 (abnormal closure): unexpected EOF socketmode: 2022/11/09 21:28:18 socket_mode_managed_conn.go:167: Reconnecting due to ping timeout: Slack did not send us WebSocket PING for more than Client.maxInterval socketmode: 2022/11/09 21:30:43 socket_mode_managed_conn.go:167: Reconnecting due to websocket: close 1006 (abnormal closure): unexpected EOF socketmode: 2022/11/09 21:55:46 socket_mode_managed_conn.go:167: Reconnecting due to websocket: close 1006 (abnormal closure): unexpected EOF socketmode: 2022/11/09 22:22:03 socket_mode_managed_conn.go:167: Reconnecting due to websocket: close 1006 (abnormal closure): unexpected EOF socketmode: 2022/11/09 22:36:36 socket_mode_managed_conn.go:167: Reconnecting due to websocket: close 1006 (abnormal closure): unexpected EOF socketmode: 2022/11/09 22:42:32 socket_mode_managed_conn.go:167: Reconnecting due to websocket: close 1006 (abnormal closure): unexpected EOF socketmode: 2022/11/09 23:10:13 socket_mode_managed_conn.go:167: Reconnecting due to websocket: close 1006 (abnormal closure): unexpected EOF socketmode: 2022/11/09 23:28:23 socket_mode_managed_conn.go:167: Reconnecting due to ping timeout: Slack did not send us WebSocket PING for more than Client.maxInterval socketmode: 2022/11/09 23:30:28 socket_mode_managed_conn.go:167: Reconnecting due to websocket: close 1006 (abnormal closure): unexpected EOF socketmode: 2022/11/09 23:57:20 socket_mode_managed_conn.go:167: Reconnecting due to ping timeout: Slack did not send us WebSocket PING for more than Client.maxInterval socketmode: 2022/11/10 00:10:58 socket_mode_managed_conn.go:167: Reconnecting due to websocket: close 1006 (abnormal closure): unexpected EOF socketmode: 2022/11/10 00:51:08 socket_mode_managed_conn.go:167: Reconnecting due to websocket: close 1006 (abnormal closure): unexpected EOF socketmode: 2022/11/10 01:14:14 socket_mode_managed_conn.go:167: Reconnecting due to websocket: close 1006 (abnormal closure): unexpected EOF

I creates Client with below options socketmode.OptionDebug(true), socketmode.OptionPingInterval(60*time.Second), socketmode.OptionLog(log.New(os.Stdout, "socketmode: ", log.Lshortfile|log.LstdFlags) if I don't set OptionPingInterval, ping timeout happens every a few minutes.

lololozhkin commented 1 year ago

Facing the same issue :(. I think awaiting of this goroutine will solve the problem)

Yes, we need to wait for reading json from websocket, that can be too slow, but we can also run reading json asyncroniously and watch on ctx.Done() in select. I could implement it in PR, can you give me some feedback?

now it is written in socket_mode_managed_conn.go:442

event := json.RawMessage{}
err := conn.ReadJSON(&event)

But it could be something like that

event := json.RawMessage{}

readJsonErr := make(chan error)
go func() {
    readJsonErr <- conn.ReadJSON(&event)
}()

select {
case <-ctx.Done():
    err = conn.Close() // we need to close connection to websocket
    if err != nil {
    // ....
    }
    return ctx.Error()
case err <- readJsonErr:
    // future error handling
}

What do you think? In this case we could cancel awaiting of json read and wait for goroutine in main cycle.

lololozhkin commented 1 year ago

Created PR to resolve this problem, waiting for review and merging.

kanata2 commented 1 year ago

Thanks! I'll review later.