-
Notifications
You must be signed in to change notification settings - Fork 16
feat(ogmios): allow timeout on chain context #162
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds timeout support to the OgmiosChainContext by introducing two new configurable fields: BaseContext (for custom base contexts) and RequestTimeout (for per-request timeouts). This addresses issue #106 by allowing callers to control request timeouts and cancellation behavior.
Key changes:
- Added
BaseContextandRequestTimeoutfields to theOgmiosChainContextstruct - Updated 8 methods to support configurable base context and per-request timeouts
- Implemented timeout handling pattern using
context.WithTimeoutacross most methods
Comments suppressed due to low confidence (1)
txBuilding/Backend/OgmiosChainContext/OgmiosChainContext.go:220
- The timeout handling is missing for this method. While the BaseContext is properly applied, the RequestTimeout is not being used. This is inconsistent with other methods in the same file like TxOuts, LatestBlock, LatestEpoch, AddressUtxos, SubmitTx, and EvaluateTx, which all implement timeout handling.
Consider adding the same timeout pattern used in other methods:
- Create reqCtx from ctx
- Apply context.WithTimeout if RequestTimeout is set
- Call cancel() after the request completes
var ctx context.Context
if occ.BaseContext != nil {
ctx = occ.BaseContext
} else {
ctx = context.Background()
}
utxos, err := occ.ogmigo.UtxosByTxIn(ctx, chainsync.TxInQuery{
Transaction: shared.UtxoTxID{
ID: txHash,
},
Index: uint32(index),
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If RequestTimeout is set, derive a context with timeout for this request | ||
| reqCtx := ctx | ||
| var cancel func() | ||
| if occ.RequestTimeout > 0 { | ||
| var c context.CancelFunc | ||
| reqCtx, c = context.WithTimeout(ctx, occ.RequestTimeout) | ||
| cancel = func() { c() } | ||
| } else { | ||
| cancel = func() {} | ||
| } | ||
| us, err := occ.ogmigo.UtxosByTxIn(reqCtx, queries...) | ||
| cancel() |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeout handling logic is duplicated across multiple methods (TxOuts, LatestBlock, LatestEpoch, AddressUtxos, LatestEpochParams, SubmitTx, EvaluateTx). This pattern of creating reqCtx, cancel function, and calling cancel after the request is repeated identically.
Consider extracting this into a helper method to reduce duplication and improve maintainability. For example:
func (occ *OgmiosChainContext) getRequestContext(baseCtx context.Context) (context.Context, func())
This would return the appropriate context and cancel function based on whether RequestTimeout is set.
| var ctx context.Context | ||
| if occ.BaseContext != nil { | ||
| ctx = occ.BaseContext | ||
| } else { | ||
| ctx = context.Background() | ||
| } |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base context initialization pattern (checking if BaseContext is nil and falling back to context.Background()) is duplicated across all methods. This adds unnecessary verbosity to the code.
Consider extracting this into a helper method to reduce duplication:
func (occ *OgmiosChainContext) getBaseContext() context.Context
This would centralize the logic and make the code more maintainable.
| var cancel func() | ||
| if occ.RequestTimeout > 0 { | ||
| var c context.CancelFunc | ||
| reqCtx, c = context.WithTimeout(ctx, occ.RequestTimeout) | ||
| cancel = func() { c() } | ||
| } else { | ||
| cancel = func() {} | ||
| } | ||
| us, err := occ.ogmigo.UtxosByTxIn(reqCtx, queries...) | ||
| cancel() |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a wrapper function around the cancel function (cancel = func() { c() }) is unnecessary. The CancelFunc returned by context.WithTimeout can be assigned directly to cancel. Additionally, the else branch creating an empty no-op function is not needed since calling a nil function is handled by the defer pattern, or you can simply check if cancel != nil before calling it.
Consider simplifying to:
- Assign c directly to cancel without wrapping
- Either remove the else branch or check if cancel != nil before calling it
| var cancel func() | |
| if occ.RequestTimeout > 0 { | |
| var c context.CancelFunc | |
| reqCtx, c = context.WithTimeout(ctx, occ.RequestTimeout) | |
| cancel = func() { c() } | |
| } else { | |
| cancel = func() {} | |
| } | |
| us, err := occ.ogmigo.UtxosByTxIn(reqCtx, queries...) | |
| cancel() | |
| var cancel context.CancelFunc | |
| if occ.RequestTimeout > 0 { | |
| reqCtx, cancel = context.WithTimeout(ctx, occ.RequestTimeout) | |
| } | |
| us, err := occ.ogmigo.UtxosByTxIn(reqCtx, queries...) | |
| if cancel != nil { | |
| cancel() | |
| } |
| // Optional base context used for requests. If nil, context.Background() is used. | ||
| BaseContext context.Context | ||
| // Optional per-request timeout. If zero, no timeout is applied. |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BaseContext and RequestTimeout fields are exported (public) but there's no validation or guidance on their usage. Consider documenting:
- Whether BaseContext can be cancelled externally and what happens if it is
- The interaction between BaseContext cancellation and RequestTimeout
- Thread-safety considerations if these fields are modified after the struct is in use
Additionally, consider whether these fields should be set via functional options in the constructor rather than being directly mutable public fields, which would be more idiomatic Go and prevent potential issues with concurrent access.
| // Optional base context used for requests. If nil, context.Background() is used. | |
| BaseContext context.Context | |
| // Optional per-request timeout. If zero, no timeout is applied. | |
| // BaseContext is an optional parent context used for all outgoing requests. | |
| // | |
| // - If BaseContext is nil, context.Background() is used as the parent. | |
| // - If BaseContext is cancelled externally, callers should expect in-flight | |
| // and future requests that derive from it to be cancelled as well, typically | |
| // resulting in context.Canceled or context.DeadlineExceeded errors. | |
| // - BaseContext is configuration and is not safe to modify concurrently with | |
| // active use of OgmiosChainContext; it should be set once at construction | |
| // time and treated as read-only thereafter. | |
| BaseContext context.Context | |
| // RequestTimeout is an optional per-request timeout applied on top of | |
| // BaseContext (or context.Background() if BaseContext is nil). | |
| // | |
| // - If RequestTimeout is greater than zero, each request should derive a | |
| // child context with the given timeout; cancellation of either the timeout | |
| // or BaseContext will cancel the request. | |
| // - If RequestTimeout is zero, no additional per-request deadline is applied | |
| // beyond any deadline already present on BaseContext. | |
| // - RequestTimeout is configuration and is not safe to modify concurrently | |
| // with active use of OgmiosChainContext; it should be set once at | |
| // construction time and treated as read-only thereafter. |
Signed-off-by: Chris Gianelloni <[email protected]>
4e02906 to
3d51d6e
Compare
Closes #106