feat(schema/appdata): async listener mux'ing #20879
Conversation
WalkthroughWalkthroughThe recent changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AsyncListenerMux
participant Listener1
participant Listener2
Client->>AsyncListenerMux: Send Event
AsyncListenerMux->>Listener1: Forward Event
AsyncListenerMux->>Listener2: Forward Event
Listener1->>AsyncListenerMux: Acknowledge Event
Listener2->>AsyncListenerMux: Acknowledge Event
AsyncListenerMux->>Client: Confirm Event Handling
sequenceDiagram
participant Client
participant AsyncListener
participant EventProcessor
Client->>AsyncListener: Send Event
AsyncListener->>EventProcessor: Process Event (goroutine)
EventProcessor->>AsyncListener: Event Result
AsyncListener->>Client: Confirm Event Handling
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@aaronc your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (3)
schema/appdata/mux.go (3)
3-4: Ensure documentation clarity.The comments should clarify that the returned listener will forward events to all provided listeners and that callbacks will only be registered if at least one listener has a non-nil callback.
- // ListenerMux returns a listener that forwards received events to all the provided listeners in order. - // A callback is only registered if a non-nil callback is present in at least one of the listeners. + // ListenerMux returns a listener that forwards received events to all the provided listeners in order. + // A callback is only registered if a non-nil callback is present in at least one of the listeners. + // The returned listener will forward events to all provided listeners.
5-7: Initializemuxwith default values.Ensure that
muxis correctly initialized with default values for all possible callback functions to prevent nil pointer dereference.
schema/appdata/mux.go, functionListenerMux, initializemuxwith default no-op functions for all callbacks.Analysis chain
Initialize
muxwith default values.Ensure that
muxis correctly initialized with default values for all possible callback functions to prevent nil pointer dereference.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all callback functions in Listener are initialized with default values. # Test: Search for the Listener struct definition. Expect: All callback functions are initialized. rg --type go -A 10 'type Listener struct'Length of output: 1057
Script:
#!/bin/bash # Description: Retrieve the complete definition of the Listener struct to identify all callback functions. rg --type go -A 20 'type Listener struct' schema/appdata/listener.goLength of output: 1145
Script:
#!/bin/bash # Description: Check the ListenerMux function implementation to see how mux is initialized. rg --type go -A 10 'func ListenerMux' schema/appdata/mux.goLength of output: 433
120-121: Ensure default initialization for all callbacks.The
Listenerstruct is instantiated without initializing its callback functions in the following locations:
schema/appdata/mux.go:mux := Listener{}schema/appdata/async.go:res := Listener{}Ensure that all possible callbacks in the returned
Listenerare initialized with default values to prevent potential nil pointer dereferences.Analysis chain
Ensure default initialization for all callbacks.
Ensure that all possible callbacks in the returned listener are initialized with default values.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all callbacks in the returned Listener are initialized with default values. # Test: Search for the Listener struct definition. Expect: All callback functions are initialized. rg --type go -A 10 'type Listener struct'Length of output: 1057
Script:
#!/bin/bash # Description: Extract the complete definition of the Listener struct to verify initialization of all callback functions. # Search for the complete definition of the Listener struct. rg --type go -A 30 'type Listener struct' schema/appdata/listener.goLength of output: 1804
Script:
#!/bin/bash # Description: Search for instances where the Listener struct is instantiated and verify initialization of callbacks. # Find instances of Listener struct instantiation. rg --type go 'Listener{'Length of output: 108
Script:
#!/bin/bash # Description: Search for instances where the Listener struct is instantiated and verify initialization of callbacks. # Find instances of Listener struct instantiation. rg --type go 'Listener\{'Length of output: 1191
Script:
#!/bin/bash # Description: Extract context around the instantiation of Listener to verify initialization of callbacks. # Extract context around the instantiation in mux.go rg --type go -A 10 -B 10 'Listener\{\}' schema/appdata/mux.go # Extract context around the instantiation in async.go rg --type go -A 10 -B 10 'Listener\{\}' schema/appdata/async.goLength of output: 1904
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- schema/appdata/async.go (1 hunks)
- schema/appdata/async_test.go (1 hunks)
- schema/appdata/mux.go (1 hunks)
- schema/appdata/mux_test.go (1 hunks)
Additional context used
Path-based instructions (4)
schema/appdata/mux.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/appdata/mux_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"schema/appdata/async_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"schema/appdata/async.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (12)
schema/appdata/mux_test.go (5)
8-33: Ensure comprehensive test coverage.The "empty" sub-test verifies that the returned listener has nil callbacks when no listeners are provided. This is a good basic test, but consider adding more edge cases.
35-61: Ensure test order reflects realistic usage.The "all called once" sub-test verifies that all callbacks are called in order. This test is thorough and covers realistic usage scenarios.
64-86: Ensure comprehensive callback testing.The
callAllCallbacksOncesfunction ensures that all callbacks are called once. This is a good utility function for testing.
88-119: Ensure accurate callback simulation.The
callCollectorfunction simulates callbacks and collects call information. This is a useful utility for testing.
121-131: Ensure accurate call order verification.The
checkExpectedCallOrderfunction verifies the order of callback invocations. This is a crucial utility for ensuring the correctness of callback ordering.schema/appdata/async_test.go (5)
10-34: Ensure comprehensive test coverage.The "empty" sub-test verifies that the returned listener has nil callbacks when no listeners are provided. This is a good basic test, but consider adding more edge cases.
36-68: Ensure context cancellation is handled correctly.The "call cancel" sub-test verifies that all callbacks are called and the context is cancelled correctly. This test is thorough and covers realistic usage scenarios.
70-87: Ensure error handling in commit is tested.The "error on commit" sub-test verifies error handling during commit. This is a crucial test for ensuring robustness.
90-124: Ensure context cancellation is handled correctly.The "call cancel" sub-test verifies that all callbacks are called and the context is cancelled correctly. This test is thorough and covers realistic usage scenarios.
126-147: Ensure error handling in callbacks is tested.The "error" sub-test verifies error handling during callback execution. This is a crucial test for ensuring robustness.
schema/appdata/async.go (2)
8-19: Ensure comprehensive documentation for struct fields.The
AsyncListenerOptionsstruct defines options for async listeners and listener mux's. The documentation should ensure that all fields are clearly explained.
22-55: Ensure proper handling of asynchronous listeners.The
AsyncListenerMuxfunction manages asynchronous listeners and ensures that all commit errors are handled correctly. This implementation is crucial for maintaining robustness in concurrent contexts.
| packetChan <- data | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
res.callbacks := make(func(data Packet) error, 0)How about having a general callback slice instead of all individual callbacks?
There was a problem hiding this comment.
I'm not sure I follow how this would work. Could you explain more?
There was a problem hiding this comment.
IMO, we can keep only one callback function within Listener, since all callbacks StartBlock, OnTx ... are in the same format as func(packet Packet) error. And then within the callback implementation, we can deal with different packet type like ModuleInitializationData, StartBlockData, and so on using a switch statement. It will reduce the duplicated imp here, and in mux.go
There was a problem hiding this comment.
We already defined the different packet data structures, so we don't need to distinguish the callback functions.
There was a problem hiding this comment.
The purpose of distinguishing the different callback functions is to signal what each listener is subscribing to. If listeners don't listen to events at all, then OnEvent should be nil and then the producers won't send any events. If there's only one callback function then a producer has to always send all packets even if the consumers are only interested in certain packet types. In the async case it simplifies what is sent over each packet channel to only the subscription callbacks that are non nil.
Co-authored-by: cool-developer <51834436+cool-develope@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- schema/appdata/mux.go (1 hunks)
Additional context used
Path-based instructions (1)
schema/appdata/mux.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (8)
schema/appdata/mux.go (8)
1-23: LGTM! Initialization andInitializeModuleDatahandling are correct.The initialization of the
Listenerstruct and the handling ofInitializeModuleDatacallbacks are well-implemented.
25-40: LGTM!StartBlockhandling is correct.The handling of
StartBlockcallbacks is well-implemented.
42-57: LGTM!OnTxhandling is correct.The handling of
OnTxcallbacks is well-implemented.
59-74: LGTM!OnEventhandling is correct.The handling of
OnEventcallbacks is well-implemented.
76-91: LGTM!OnKVPairhandling is correct.The handling of
OnKVPaircallbacks is well-implemented.
93-108: LGTM!OnObjectUpdatehandling is correct.The handling of
OnObjectUpdatecallbacks is well-implemented.
110-125: LGTM!Commithandling is correct.The handling of
Commitcallbacks is well-implemented.
127-128: LGTM! Return statement is correct.The return statement correctly returns the initialized
Listenerstruct.
* main: feat(log): remove core dependency and update core interface to be dependency free (#21045) chore: fix some comments (#21085) feat: simulate nested messages (#20291) chore(network): remove `DefaultConfigWithAppConfigWithQueryGasLimit` (#21055) fix(runtime): remove `appv1alpha1.Config` from runtime (#21042) feat(simapp/v2): Add store server to testnet init cmd (#21076) chore(indexer/postgres): update to changes on main (#21077) feat(schema/appdata): async listener mux'ing (#20879) ci: Use large box for 052 branch sims on CI (#21067) chore(all): replace all `fmt.Errorf` without paramters with `errors.New` (#21068)
Description
This PR is part of the implementation of #20647. It introduces
appdata.Listenerutilities for:Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Tests