Skip to content

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

Merged
hongalex merged 3 commits into
googleapis:mainfrom
zchee:fix-defer-createSpan
Dec 6, 2024
Merged

fix(pubsub): fix defer call in for loop#11175
hongalex merged 3 commits into
googleapis:mainfrom
zchee:fix-defer-createSpan

Conversation

@zchee

@zchee zchee commented Nov 22, 2024

Copy link
Copy Markdown
Contributor

SSIA. Avoid the defer function call in for loop.

@zchee
zchee requested a review from shollyman as a code owner November 22, 2024 13:51
@zchee
zchee requested review from a team 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

zchee commented Nov 22, 2024

Copy link
Copy Markdown
Contributor Author

/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

zchee commented Dec 5, 2024

Copy link
Copy Markdown
Contributor Author

@hongalex gentle ping

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

hongalex commented Dec 5, 2024

Copy link
Copy Markdown
Member

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

zchee commented Dec 5, 2024

Copy link
Copy Markdown
Contributor Author

@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.

Comment thread pubsub/topic.go Outdated
@hongalex

hongalex commented Dec 5, 2024

Copy link
Copy Markdown
Member

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

zchee commented Dec 5, 2024

Copy link
Copy Markdown
Contributor Author

@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

hongalex commented Dec 6, 2024

Copy link
Copy Markdown
Member

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
@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