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

MM-46402: Use pointer to pointer to set pointer to nil #20827

Merged
merged 5 commits into from Aug 19, 2022

Conversation

ashishbhate
Copy link
Contributor

@ashishbhate ashishbhate commented Aug 16, 2022

Summary

  • We need a pointer to a pointer to set the original pointer to nil
  • The original task was not being set to nil (the local variable containing the pointer was being set to nil). The cancel func was being called even though the task had already been cancelled. This repeated cancellation was causing a panic because we were trying to close a channel that had already been closed the first time the task was cancelled.

Ticket Link

Release Note

NONE

@mm-cloud-bot
Copy link

@ashishbhate: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

mut.Lock()
defer mut.Unlock()
if task != nil {
task.Cancel()
task = nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here just the local variable (task) containing the pointer was being set to nil.
We needed to set the pointer of the global task variable (a.ch.dndTask or a.ch.postReminderTask, depending on where this function was called) to nil.

Copy link
Member

@agarciamontoro agarciamontoro left a comment

Choose a reason for hiding this comment

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

Aaaaah, this makes sense! Great catch, @ashishbhate, I completely missed this when first looking at the code. Thank you for the fix :)

I left a couple of optional comments below.

app/server_test.go Show resolved Hide resolved
app/server.go Outdated Show resolved Hide resolved
@ashishbhate ashishbhate changed the title Use Pointer to pointer to set pointer to nil MM-46402: Use pointer to pointer to set pointer to nil Aug 16, 2022
app/server.go Outdated Show resolved Hide resolved
Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

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

Nice!

@ashishbhate
Copy link
Contributor Author

/e2e-test

@mattermod
Copy link
Contributor

@ashishbhate
Copy link
Contributor Author

@agarciamontoro I'm having trouble connecting to our internal servers. If the report is passing, could you please merge this PR?

@agarciamontoro
Copy link
Member

If the report is passing, could you please merge this PR?

@ashishbhate It looks like it's not, it's failing in the report step. I'm not very familiar with these e2e tests yet. @agnivade, what's the usual process here?

@agnivade
Copy link
Member

https://community.mattermost.com/private-core/pl/xwygwzxgdpgybpfoprfjy6wpjh

You'll need to compare the failures against a recent master test run. Looking at the results, they look expected to me. Failures are unrelated.

@ashishbhate ashishbhate merged commit ff4406e into master Aug 19, 2022
@ashishbhate ashishbhate deleted the pointers-all-the-way-down branch August 19, 2022 01:57
@amyblais
Copy link
Member

/cherry-pick cloud

@mattermod
Copy link
Contributor

Cherry pick is scheduled.

@mattermod mattermod added the CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone label Aug 19, 2022
mattermost-build pushed a commit to mattermost-build/mattermost that referenced this pull request Aug 19, 2022
)

Summary
We need a pointer to a pointer to set the original pointer to nil
The original task was not being set to nil (the local variable containing the pointer was being set to nil). The cancel function was being called even though the task had already been cancelled. This repeated cancellation was causing a panic because we were trying to close a channel that had already been closed the first time the task was cancelled.

Ticket Link
https://mattermost.atlassian.net/browse/MM-46402

(cherry picked from commit ff4406e)
@amyblais amyblais added the Changelog/Not Needed Does not require a changelog entry label Aug 19, 2022
@amyblais amyblais added the Docs/Not Needed Does not require documentation label Aug 19, 2022
amyblais pushed a commit that referenced this pull request Aug 19, 2022
Summary
We need a pointer to a pointer to set the original pointer to nil
The original task was not being set to nil (the local variable containing the pointer was being set to nil). The cancel function was being called even though the task had already been cancelled. This repeated cancellation was causing a panic because we were trying to close a channel that had already been closed the first time the task was cancelled.

Ticket Link
https://mattermost.atlassian.net/browse/MM-46402

(cherry picked from commit ff4406e)

Co-authored-by: Ashish Bhate <ashish.bhate@mattermost.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog/Not Needed Does not require a changelog entry CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants