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

lock during HandleOobm #21934

Open
wants to merge 1 commit into
base: master
from
Open

lock during HandleOobm #21934

wants to merge 1 commit into from

Conversation

@joshblum
Copy link
Member

joshblum commented Jan 7, 2020

No description provided.

@joshblum joshblum requested a review from mmaxim Jan 7, 2020
@AMarcedone

This comment has been minimized.

Copy link
Contributor

AMarcedone commented Jan 7, 2020

That fixed it! Thanks a lot!

@@ -1314,21 +1314,20 @@ func (g *PushHandler) SubteamRename(ctx context.Context, m gregor.OutOfBandMessa
}

func (g *PushHandler) HandleOobm(ctx context.Context, obm gregor.OutOfBandMessage) (bool, error) {
// Don't process messages if we have not started.
g.Lock()

This comment has been minimized.

Copy link
@mmaxim

mmaxim Jan 7, 2020

Contributor

Why does it hold this lock the whole function call?

This comment has been minimized.

Copy link
@joshblum

joshblum Jan 7, 2020

Author Member

holds the lock while we register the function with the error group

This comment has been minimized.

Copy link
@mmaxim

mmaxim Jan 8, 2020

Contributor

I think this is dangerous, what is this actually solving for?

This comment has been minimized.

Copy link
@joshblum

joshblum Jan 8, 2020

Author Member

All of these calls just decode and kick off a thread though, what is the concern? The fix isjust to allow concurrent test runs @AMarcedone was trying to track down a race. Otherwise the tests can panic with a log after completion

This comment has been minimized.

Copy link
@mmaxim

mmaxim Jan 8, 2020

Contributor

They also call WaitForTurn

This comment has been minimized.

Copy link
@mmaxim

mmaxim Jan 8, 2020

Contributor

Also, I've never seen a log after completion from this place, how is his test triggering it?

This comment has been minimized.

Copy link
@joshblum

joshblum Jan 8, 2020

Author Member

Log after completion happens because we're handling some OOBM after shutdown, log could happen anywhere during processing inboxsource, convsource etc

This comment has been minimized.

Copy link
@mmaxim

mmaxim Jan 8, 2020

Contributor

I dislike changing anything just to serve tests, let me look at it a little more.

This comment has been minimized.

Copy link
@joshblum

joshblum Jan 8, 2020

Author Member

ok

This comment has been minimized.

Copy link
@joshblum

joshblum Jan 15, 2020

Author Member

@mmaxim bump on this

@joshblum joshblum requested a review from mmaxim Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.