-
Notifications
You must be signed in to change notification settings - Fork 109
refactor: events refactor (Listen, Dispatch) #1133
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
base: master
Are you sure you want to change the base?
Changes from all commits
ece190e
29e27bd
4d010e7
0976bf6
567db31
cd777d8
52b46a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,40 +1,75 @@ | ||||||
| package event | ||||||
|
|
||||||
| type Instance interface { | ||||||
| // Register event listeners to the application. | ||||||
| Register(map[Event][]Listener) | ||||||
| // Job create a new event task. | ||||||
| Job(event Event, args []Arg) Task | ||||||
| // GetEvents gets all registered events. | ||||||
| GetEvents() map[Event][]Listener | ||||||
| type Arg struct { | ||||||
| Type string | ||||||
| Value any | ||||||
| } | ||||||
|
|
||||||
| type Event interface { | ||||||
| // Handle the event. | ||||||
| Handle(args []Arg) ([]Arg, error) | ||||||
| } | ||||||
|
|
||||||
| type Listener interface { | ||||||
| // Signature returns the unique identifier for the listener. | ||||||
| Signature() string | ||||||
| type EventListener interface { | ||||||
| // Handle the event. | ||||||
| Handle(event any, args ...any) error | ||||||
| } | ||||||
|
|
||||||
| type EventQueueListener interface { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
| // Queue configure the event queue options. | ||||||
| Queue(args ...any) Queue | ||||||
| // Handle the event. | ||||||
| Handle(args ...any) error | ||||||
|
|
||||||
| EventListener | ||||||
| Signature | ||||||
| ShouldQueue | ||||||
| } | ||||||
|
|
||||||
| type Task interface { | ||||||
| // Dispatch an event and call the listeners. | ||||||
| Dispatch() error | ||||||
| type Instance interface { | ||||||
| // Dispatch fires an event and calls the listeners. | ||||||
| Dispatch(event any, payload ...[]Arg) []any | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
| // GetEvents gets all registered events. | ||||||
| GetEvents() map[Event][]Listener | ||||||
| // Job create a new event task. | ||||||
| // Deprecated: Use Dispatch instead. Job will be removed in a future version. | ||||||
| Job(event Event, args []Arg) Task | ||||||
| // Listen registers an event listener with the dispatcher. | ||||||
| // events can be: string, []string, Event, []Event, or any other type | ||||||
| // listener can be: function, class, or any callable | ||||||
| Listen(events any, listener ...any) error | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new |
||||||
| // Register event listeners to the application. | ||||||
| Register(map[Event][]Listener) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add the DEPRECATED tag for this function. |
||||||
| } | ||||||
|
|
||||||
| type Arg struct { | ||||||
| Value any | ||||||
| Type string | ||||||
| type Listener interface { | ||||||
| // Handle the event. | ||||||
| Handle(args ...any) error | ||||||
| // Queue configure the event queue options. | ||||||
| Queue(args ...any) Queue | ||||||
| // Signature returns the unique identifier for the listener. | ||||||
| Signature() string | ||||||
| } | ||||||
|
|
||||||
| type Queue struct { | ||||||
| Connection string | ||||||
| Queue string | ||||||
| Enable bool | ||||||
| Queue string | ||||||
hwbrzzl marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| } | ||||||
|
|
||||||
| type QueueListener interface { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why add this function instead of using |
||||||
| Listener | ||||||
| Signature | ||||||
|
||||||
| Signature |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,22 +2,58 @@ package event | |||||||||||||||||
|
|
||||||||||||||||||
| import ( | ||||||||||||||||||
| "slices" | ||||||||||||||||||
| "sync" | ||||||||||||||||||
|
|
||||||||||||||||||
| "github.com/goravel/framework/contracts/event" | ||||||||||||||||||
| "github.com/goravel/framework/contracts/queue" | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| type Application struct { | ||||||||||||||||||
| events map[event.Event][]event.Listener | ||||||||||||||||||
| queue queue.Queue | ||||||||||||||||||
| listeners map[string][]any | ||||||||||||||||||
| mu sync.RWMutex | ||||||||||||||||||
| wildcards map[string][]any | ||||||||||||||||||
| wildcardsCache sync.Map | ||||||||||||||||||
| events map[event.Event][]event.Listener | ||||||||||||||||||
| queue queue.Queue | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| func NewApplication(queue queue.Queue) *Application { | ||||||||||||||||||
| return &Application{ | ||||||||||||||||||
| queue: queue, | ||||||||||||||||||
| listeners: make(map[string][]any), | ||||||||||||||||||
| mu: sync.RWMutex{}, | ||||||||||||||||||
|
||||||||||||||||||
| mu: sync.RWMutex{}, |
Copilot
AI
Nov 29, 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 GetEvents method returns a direct reference to the internal app.events map without any locking or defensive copying. This allows external callers to modify the map concurrently, which could cause race conditions with the Register and Job methods. Consider either:
- Protecting the access with
app.mu.RLock()and returning a copy of the map, or - Documenting that the returned map should not be modified by callers.
| return app.events | |
| app.mu.RLock() | |
| defer app.mu.RUnlock() | |
| eventsCopy := make(map[event.Event][]event.Listener, len(app.events)) | |
| for k, v := range app.events { | |
| eventsCopy[k] = v | |
| } | |
| return eventsCopy |
Copilot
AI
Nov 29, 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 Job method accesses app.events map without acquiring any lock, but this map could potentially be modified by the Register method concurrently. While the current implementation of Register doesn't seem to support concurrent modification (it only assigns to the map once), accessing a map concurrently with writes can cause a runtime panic. Consider protecting this read operation with app.mu.RLock() and app.mu.RUnlock() for thread safety, consistent with how Dispatch protects its reads.
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.
Can it be merged into
EventQueueListener? Don't find it used elsewhere.