-
-
Notifications
You must be signed in to change notification settings - Fork 587
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
fix(event-catalog): local emit
& broadcast
handlers changed from async to event-driven
#1096
base: master
Are you sure you want to change the base?
Conversation
…ally and remotely issue moleculerjs#1065" This reverts commit 0ff0f9d.
@@ -223,7 +222,8 @@ describe("Test Tracing feature with actions", () => { | |||
} | |||
}); | |||
|
|||
await Promise.delay(500); | |||
// event loop lag <10ms | |||
await Promise.delay(510); |
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.
I think this issue is the next tick in node v10, and the test fails. The timeout should be 510ms (although it works with 501ms) because the action already has a 500ms timeout.
It is likely that after v10, optimization was carried out for placing events in a queue, and on later versions, the test passed successfully with the same delay of 500 ms.
emit
& broadcast
handlers changed from async to event-drivenemit
& broadcast
handlers changed from async to event-driven
@@ -35,6 +38,20 @@ class EventCatalog { | |||
this.events = []; | |||
|
|||
this.EndpointFactory = EventEndpoint; | |||
|
|||
this.on("broker.event", ctx => { |
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.
Why we should use an event emitter to call a non-await-ed method? I feel it's too expensive. Not enough just skip the await
in the callEventHandler
🎯 Relevant issues
issue#1065
💎 Type of change
🏁 Checklist: