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.

Sign up for GitHub

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

Jump to bottom

socketmode: awaiting of message receiver goroutine in run method to avoid panics #1170

Merged
merged 1 commit into from
Apr 15, 2023
Merged

socketmode: awaiting of message receiver goroutine in run method to avoid panics #1170

merged 1 commit into from
Apr 15, 2023

Conversation

lololozhkin
Copy link
Contributor

@lololozhkin lololozhkin commented Feb 15, 2023 •
edited

socketmode: awaiting of message receiver goroutine in run method to avoid panics

PR preparation
  • Run make pr-prep from the root of the repository to run formatting, linting and tests.

@lololozhkin
Copy link
Contributor Author

resolves #1125

@lololozhkin
Copy link
Contributor Author

lololozhkin commented Feb 22, 2023 •
edited

I don't know how to write tests on such a logic. But my production application running without any panics for at least one week, using my fork.

@kanata2 kanata2 added needs review SocketMode about SocketMode bugfix [PR] bugfix labels Apr 7, 2023
kanata2
kanata2 approved these changes Apr 15, 2023
View reviewed changes
Copy link
Member

@kanata2 kanata2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@kanata2 kanata2 linked an issue Apr 15, 2023 that may be closed by this pull request
Socketmode panic, write on closed channel #1125
Closed
@kanata2 kanata2 merged commit f2673af into slack-go:master Apr 15, 2023
@kanata2 kanata2 mentioned this pull request Apr 15, 2023
Merged
parsley42
parsley42 reviewed Apr 17, 2023
View reviewed changes
Copy link
Member

@parsley42 parsley42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yah, I know it's been merged already - but thanks for doing this. I'm afraid I was the one who made the problem in the first place - but it looks like you're fixing both issues; the long timeout on failed pings, and the panic.

iaburton added a commit to iaburton/slack that referenced this pull request Apr 24, 2023
@iaburton
Merge remote-tracking branch 'upstream/master' into safer-socketmode
8c8b886 
Functionally revert slack-go#1170 as drafted approach takes correct path of
closing chan on goroutine producer side
@lololozhkin lololozhkin self-assigned this Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parsley42 parsley42 parsley42 left review comments

@kanata2 kanata2 kanata2 approved these changes

Assignees

@lololozhkin lololozhkin

Labels
bugfix [PR] bugfix SocketMode about SocketMode
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Socketmode panic, write on closed channel
3 participants
@lololozhkin @parsley42 @kanata2