refactor: EventSwitch implementation by shotonoff · Pull Request #569 · dashpay/tenderdash

shotonoff · 2023-02-01T12:48:50Z

Issue being fixed or feature implemented

Implementation events.EventSwitch has a few downsides which were eliminated with these changes.

it has listenerID argument that is required to add a listener for the event. it is redundant parameter, and in some cases lead to unexpected behaviour, like overriding listener function.
The naming convention for components and methods is slightly different and can be confusing

The main motivation was to deprecate *listenerID*, but in the process I made the component intuitive.

What was done?

Changed package name from events on eventemitter
Renamed EventSwitch on EventEmitter
Added logger to the constructor function as an optional parameter
Renamed methods
Removed listenerID

How Has This Been Tested?

Unit / E2E tests

Breaking Changes

N/A

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation

For repository code-owners and collaborators only

I have assigned this pull request to a milestone

…ename on EventEmitter, change EventEmitter api

…itcher' into remove-listener-id-from-event-switcher

lklimek

Great code, good job!

See minor comments

lklimek · 2023-02-02T15:24:40Z

libs/eventemitter/emitter.go

+	listeners := e.events[event][:]
+	e.mtx.RUnlock()
+	for _, listener := range listeners {
+		err := listener(data)


Consider either:

adding a timeout here, to detect deadlocks, or

documenting that the listener is responsible for correct timeout handling

since the listeners' execution is synchronous, then the listener itself has to be responsible for managing concurrent behavior.
as an option, we can add the ability of asynchronous execution, but not sure we need this at this moment.

lklimek · 2023-02-02T15:30:11Z

libs/eventemitter/emitter.go

+	for _, listener := range listeners {
+		err := listener(data)
+		if err != nil {
+			e.logger.Error("got error from a listener", "error", err)


can we display name of the listener, to add some debugging info (replacing listener ID)? Maybe fmt.Sprintf("%T", listener) will do the job?

since listenerID is deprecated, we don't have any information about the listener itself.
I believe, the error must provide all the necessary information to identify the problem

shotonoff

answered

shotonoff · 2023-02-02T15:57:42Z

libs/eventemitter/emitter.go

+	listeners := e.events[event][:]
+	e.mtx.RUnlock()
+	for _, listener := range listeners {
+		err := listener(data)


since the listeners' execution is synchronous, then the listener itself has to be responsible for managing concurrent behavior.
as an option, we can add the ability of asynchronous execution, but not sure we need this at this moment.

shotonoff · 2023-02-02T16:01:10Z

libs/eventemitter/emitter.go

+	for _, listener := range listeners {
+		err := listener(data)
+		if err != nil {
+			e.logger.Error("got error from a listener", "error", err)


since listenerID is deprecated, we don't have any information about the listener itself.
I believe, the error must provide all the necessary information to identify the problem

…from-event-switcher

* refactor: add sendMessage method to consensus.State * refactor: modify sendMessage * refactor: decompose WAL interface on a few small * refactor: refactor an approach of message consuming for consensus state component * refactor: add graceful stop for chanMsgReader * refactor: consensus state priv-validator * refactor: huge refactoring of consensus state, separate each state change task as a separate command, similar to state machine implementation * refactor: add a stop function to a consensus to be able to stop receiveRoutine by a condition * fix: remove block validating in finalizeCommit * refactor: define Executor and VoteExtender interfaces, add executor mock, refactor return statement of Executor.ExtendVote * test: add TestChanQueue, TestChanMsgSender, TestChanMsgReader * test: generate PrivValidator mock * refactor: move sending stats data from message handler * refactor: add-vote operation (#558) * refactor: separate TryAddVote.addVote logic on small function with one responsibility, use 2 sets (prevote and precommit) to process adding of vote * refactor: replace peerID on proTxHash for peer catchup rounds in HeightVoteSet component * fix: remove unused errors and StateData.validateVote * refactor: EventSwitch implementation (#569) * refactor: package lib/events rename on eventemitter and EventSwitch rename on EventEmitter, change EventEmitter api * fix: add PeerManager.Unsubscribe method (#594) * refactor: set and decide proposal operations (#565) * refactor: set and decide proposal operations move to the separated component proposaler * refactor: consensus peer gossip mechanism (#551) * refactor: separate a gossip logic on implementation and a standard way of iterative handling a gossip condition * chore: remove unless commented line * refactor: remove custom "peerSyncErr" error * refactor: add Gossiper interface, generate Gossiper mock struct, add unit test to queryMaj23GossipHandler * refactor: peerGossipWorker implements service.Service * refactor: blocksync module is refactored on using worker-pool * test: replace custom assertError on tmrequire.Error * refactor: add Now function into flowrate package, to obtain the current time * fix: TestHandshakeErrorsIfAppReturnsWrongAppHash in replay_test.go * refactor: move the logic wait-for blockchain sync from reactor to BlockPool * fix: TestReactor_SyncTime * test: change timing for test TestBlockPoolBasic * chore: replace clock on clockwork * refactor: replace reactor's consuming on separate consumer function in Channel, rename SetPeerRange on AddPeer * test: add TestConsumeError * chore: remove unused mustHexToBytes * chore: add examples in promise/example_test.go and docs comments * chore: change a package for MockValidatorSet from types on factory * Update internal/blocksync/channel.go * feat: create p2p/client package (#602) * refactor: add internal/p2p/client implementation * refactor: change blocksync module to support p2p client * test: fix TestMakeHTTPDialerURL (#605) * refactor: rename RandomNode on AnyNode as more specific name * refactor: introduce p2p client in mempool module (#606)

refactor: package lib/events rename on eventemitter and EventSwitch r…

b0be326

…ename on EventEmitter, change EventEmitter api

shotonoff requested review from QuantumExplorer and lklimek as code owners February 1, 2023 12:48

shotonoff added 4 commits February 1, 2023 14:55

refactor: some minor improvement

028344c

fix: cs issues

b36ea0b

fix: cs issues

5c773a6

Merge remote-tracking branch 'origin/remove-listener-id-from-event-sw…

bab1367

…itcher' into remove-listener-id-from-event-switcher

lklimek approved these changes Feb 2, 2023

View reviewed changes

shotonoff commented Feb 2, 2023

View reviewed changes

shotonoff added 3 commits February 2, 2023 17:21

Merge branch 'td-115-fix-consensus-deadlock' into remove-listener-id-…

f1740de

…from-event-switcher

Merge branch 'td-115-fix-consensus-deadlock' into remove-listener-id-…

fc77038

…from-event-switcher

fix: cs issue

8e8fc81

shotonoff merged commit a4376b3 into td-115-fix-consensus-deadlock Feb 10, 2023

shotonoff deleted the remove-listener-id-from-event-switcher branch February 10, 2023 08:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: EventSwitch implementation#569

refactor: EventSwitch implementation#569
shotonoff merged 8 commits intotd-115-fix-consensus-deadlockfrom
remove-listener-id-from-event-switcher

shotonoff commented Feb 1, 2023 •

edited

Loading

Uh oh!

lklimek left a comment

Uh oh!

lklimek Feb 2, 2023

Uh oh!

shotonoff Feb 2, 2023

Uh oh!

lklimek Feb 2, 2023

Uh oh!

shotonoff Feb 2, 2023

Uh oh!

shotonoff left a comment

Uh oh!

shotonoff Feb 2, 2023

Uh oh!

shotonoff Feb 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

shotonoff commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

lklimek left a comment

Choose a reason for hiding this comment

Uh oh!

lklimek Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

shotonoff Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

lklimek Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

shotonoff Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

shotonoff left a comment

Choose a reason for hiding this comment

Uh oh!

shotonoff Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

shotonoff Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shotonoff commented Feb 1, 2023 •

edited

Loading