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

fix(pubsub): fix defer call in for loop #11175

Merged
merged 3 commits into from
Dec 6, 2024

Conversation

zchee
Copy link
Contributor

@zchee zchee commented Nov 22, 2024

SSIA. Avoid the defer function call in for loop.

@zchee zchee requested review from shollyman and a team as code owners November 22, 2024 13:51
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the Pub/Sub API. label Nov 22, 2024
@zchee
Copy link
Contributor Author

zchee commented Nov 22, 2024

/cc @hongalex

@zchee zchee force-pushed the fix-defer-createSpan branch from 26bbf75 to cf474ab Compare November 22, 2024 18:49
@zchee zchee changed the title pubsub: fix defer call in for loop fix(pubsub): fix defer call in for loop Nov 22, 2024
@zchee zchee force-pushed the fix-defer-createSpan branch 2 times, most recently from bc6f17c to 70dd1ef Compare November 22, 2024 22:04
@zchee
Copy link
Contributor Author

zchee commented Dec 5, 2024

@hongalex gentle ping

@zchee zchee force-pushed the fix-defer-createSpan branch from 70dd1ef to 4aea6cf Compare December 5, 2024 20:46
@hongalex
Copy link
Member

hongalex commented Dec 5, 2024

Apologies for missing this the first time around. Could you explain in more detail why this is necessary? Calling defer in a for loop can lead to issues with freeing memory, but in this case, I believe the behavior is correct and wrapping it in anonymous function makes it harder to read.

@zchee
Copy link
Contributor Author

zchee commented Dec 5, 2024

@hongalex Example playground is here: https://go.dev/play/p/tM6Gt11reCo

I thought you assumed deferAnonFunc behavior, but actually when calling defer within a for loop, added functions to the defer stack are called in reverse order. (deferForLoop behavior
If we want to keep the order of bmsgs (which I think we should), we should enclose it in an anonymous function.

pubsub/topic.go Outdated Show resolved Hide resolved
@hongalex
Copy link
Member

hongalex commented Dec 5, 2024

Yeah I definitely overlooked the order of when the message spans are ended when using defer. In my mind, it's not a super pressing bug since the events are batched anyway on the exporter so the timing is fairly tight, but it would be nice to get correct.

If that's the case, I would prefer iterating over bmsgs rather than allocating space for the function calls. wdyt?

t.publishMessageBundle(ctx, bmsgs)
if t.enableTracing {
	for i, m := range bmsgs {
		m.createSpan.End()
		m.createSpan.AddEvent(eventPublishEnd)
        }
}

@zchee
Copy link
Contributor Author

zchee commented Dec 5, 2024

@hongalex I agreed your opinion.

But just for confirm, you means calling AddEvent before End instead of?

t.publishMessageBundle(ctx, bmsgs)
if t.enableTracing {
	for _, m := range bmsgs {
		m.createSpan.AddEvent(eventPublishEnd)
		m.createSpan.End()
    }
}

Edit: changed and pushed. PTAL.

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@hongalex
Copy link
Member

hongalex commented Dec 6, 2024

But just for confirm, you means calling AddEvent before End instead of?

Sorry, no I didn't. I copy and pasted the order from the defer side (because the opposite order is correct) but in this case we should be calling AddEvent and then End.
Thanks!

@hongalex hongalex added the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 6, 2024
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 6, 2024
@hongalex hongalex added the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 6, 2024
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 6, 2024
@hongalex hongalex enabled auto-merge (squash) December 6, 2024 18:03
@hongalex hongalex added the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 6, 2024
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 6, 2024
@hongalex hongalex merged commit 7aec711 into googleapis:main Dec 6, 2024
5 checks passed
@zchee zchee deleted the fix-defer-createSpan branch December 6, 2024 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants