Skip to content

add ctx to blocksItr#88

Merged
liran-funaro merged 1 commit intohyperledger:mainfrom
cendhu:blk-itr-ctx
Mar 3, 2026
Merged

add ctx to blocksItr#88
liran-funaro merged 1 commit intohyperledger:mainfrom
cendhu:blk-itr-ctx

Conversation

@cendhu
Copy link
Contributor

@cendhu cendhu commented Mar 2, 2026

Type of change

  • Bug fix
  • Improvement (improvement to code, performance, etc)

Description

Add context.Context to the block iterator so that waitForBlock respects context cancellation. This fixes goroutine leaks in callers like deliver.go where cursor.Next() blocks indefinitely even after the gRPC stream's context is cancelled.

@cendhu cendhu force-pushed the blk-itr-ctx branch 3 times, most recently from a27a5e3 to 1ffbacc Compare March 3, 2026 05:04
@cendhu cendhu requested a review from liran-funaro March 3, 2026 05:16
Copy link
Contributor

@liran-funaro liran-funaro left a comment

Choose a reason for hiding this comment

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

The iterator does not have to hold the context in this case.
Instead, it.Next() should accept a context parameter, which is propagated downwards on a per call basis.
I suggest a re-work on the basis on the above claim.

@cendhu
Copy link
Contributor Author

cendhu commented Mar 3, 2026

The iterator does not have to hold the context in this case.
Instead, it.Next() should accept a context parameter, which is propagated downwards on a per call basis.
I suggest a re-work on the basis on the above claim.

I know. I went with the current design based on the following:

  1. Throughout the iterator lifetime, the ctx would not change.
  2. I did not want to make a significant changes.

I don't mind adding ctx to the Next().

@liran-funaro
Copy link
Contributor

@cendhu Will introducing context to it.Next() will result in a major diff?

@cendhu
Copy link
Contributor Author

cendhu commented Mar 3, 2026

@cendhu Will introducing context to it.Next() will result in a major diff?

I expected so but It does not seem to be true given I making the changes. In fact, lesser number of changes.

@cendhu cendhu force-pushed the blk-itr-ctx branch 3 times, most recently from 5a40e3e to 7e7cb92 Compare March 3, 2026 12:23
Add context.Context to the block iterator so that waitForBlock respects
context cancellation. This fixes goroutine leaks in callers like
deliver.go where cursor.Next() blocks indefinitely even after the gRPC
stream's context is cancelled.

Signed-off-by: Senthilnathan <cendhu@gmail.com>
@liran-funaro liran-funaro merged commit 7df0f34 into hyperledger:main Mar 3, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants