abci: Port EventAttribute field type change to main#9335
Conversation
Cherry-pick of 09a6ad7
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
| select { | ||
| case <-ctx.Done(): | ||
| break | ||
| break EQ_LOOP |
There was a problem hiding this comment.
Are we sure that, if the context passed as parameter to the function is "done", we just want to break out of the loop? (but still execute the one at the end: REMOVE_LOOP)
There was a problem hiding this comment.
Given the comment above the select statement I assumed that this is what was originally meant. Without specifying the for loop label, the existing code just breaks out of the select statement, which means we may as well just ignore the context altogether.
There was a problem hiding this comment.
The only alternative I can think of is to return from the function with an error.
What would trigger the termination of the context outside of the tests?
There was a problem hiding this comment.
Yes, I got the intention of you changes, and they are definitely improving the logic.
It's just that, if the context is "done", don't we just want to return immediately? (if we're unsure, let's just leave it as you have it)
There was a problem hiding this comment.
Yeah I think this is something we need to clean up. I doubt it's catastrophic if we don't, but it is a bit messy right now.
sergio-mena
left a comment
There was a problem hiding this comment.
Managing contexts in this part of the code, anyway, needs much more work, I don't think it should block this PR
Closes #6403.
This PR is a port of #6408 to
mainfor release in v0.37.PR checklist
CHANGELOG_PENDING.mdupdated, or no changelog entry neededdocs/) and code comments, or nodocumentation updates needed