Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Deadlock when emitting and processing events #553

Closed
ankur22 opened this issue Sep 29, 2022 · 0 comments · Fixed by #555
Closed

Deadlock when emitting and processing events #553

ankur22 opened this issue Sep 29, 2022 · 0 comments · Fixed by #555
Assignees
@ankur22
Labels
bug Something isn't working events CDP or internal events related. next Might be eligible for the next planning (not guaranteed!)
Milestone
v0.6.0

Comments

@ankur22
Copy link
Collaborator

ankur22 commented Sep 29, 2022

The ordering of events that are emitted is sometimes lost due to the use of the goroutine during the emit to the handlers.

This was found to eventually cause a deadlock in cloud runs.

To overcome this issue we tried to remove the goroutine, but the use of buffered channels didn't help in reducing the possibility of other deadlocks.

We still need to find a robust and maintainable solution whereby the order of the emitted events is somehow kept when the event is emitted to the handlers.
@ankur22 ankur22 added bug Something isn't working next Might be eligible for the next planning (not guaranteed!) labels Sep 29, 2022
@ankur22 ankur22 added this to the v0.6.0 milestone Sep 29, 2022
@ankur22 ankur22 self-assigned this Sep 29, 2022
ankur22 added a commit that referenced this issue Sep 29, 2022
@ankur22
Add a queue per handler to keep events in order
a6b8a04 
Events are sometimes out of order due to use using a goroutine to
publish events to the handler concurrently, so there's no guarantee
that the goroutine will run in the order they're created. To overcome
this, a queue has been added per handler to synchronise all publishes
of events to that handler. This should ensure that the ordering of
events consumed by the handler matches what is emitted. We can't remove
the goroutine in emitTo as this deadlocks due to handlers consuming and
emitting events.

Closes: #553
@ankur22 ankur22 mentioned this issue Sep 29, 2022
Merged
ankur22 added a commit that referenced this issue Sep 30, 2022
@ankur22
Add a queue which is read from for emitting event
b8a1d67 
Firstly we have to remove the goroutine that is causing the events to
be published out of order. Once we have done that though, we end up
deadlocking when a handler publishes and event and also is waiting to
consume an event.

To overcome this we places a queue in front of the emit function. This
will help ensure that the ordering of emitted events is kept, and also
creates a buffer allowing handlers to consume and publish events
without deadlocking.

Closes: #553
ankur22 added a commit that referenced this issue Sep 30, 2022
@ankur22
Add a queue which is read from for emitting event
328dc88 
Firstly we have to remove the goroutine that is causing the events to
be published out of order. Once we have done that though, we end up
deadlocking when a handler publishes and event and also is waiting to
consume an event.

To overcome this we places a queue in front of the emit function. This
will help ensure that the ordering of emitted events is kept, and also
creates a buffer allowing handlers to consume and publish events
without deadlocking.

Closes: #553
ankur22 added a commit that referenced this issue Oct 5, 2022
@ankur22
Add a queue per handler to keep events in order
c9bf312 
Events are sometimes out of order due to use using a goroutine to
publish events to the handler concurrently, so there's no guarantee
that the goroutine will run in the order they're created. To overcome
this, a queue has been added per handler to synchronise all publishes
of events to that handler. This should ensure that the ordering of
events consumed by the handler matches what is emitted. We can't remove
the goroutine in emitTo as this deadlocks due to handlers consuming and
emitting events.

Closes: #553
ankur22 added a commit that referenced this issue Oct 6, 2022
@ankur22
Add a queue per handler to keep events in order
d33fe66 
Events are sometimes out of order due to use using a goroutine to
publish events to the handler concurrently, so there's no guarantee
that the goroutine will run in the order they're created. To overcome
this, a queue has been added per handler to synchronise all publishes
of events to that handler. This should ensure that the ordering of
events consumed by the handler matches what is emitted. We can't remove
the goroutine in emitTo as this deadlocks due to handlers consuming and
emitting events.

Closes: #553
ankur22 added a commit that referenced this issue Oct 6, 2022
@ankur22
Add a queue per handler to keep events in order
b8d92e2 
Events are sometimes out of order due to use using a goroutine to
publish events to the handler concurrently, so there's no guarantee
that the goroutine will run in the order they're created. To overcome
this, a queue has been added per handler to synchronise all publishes
of events to that handler. This should ensure that the ordering of
events consumed by the handler matches what is emitted. We can't remove
the goroutine in emitTo as this deadlocks due to handlers consuming and
emitting events.

Closes: #553
This was referenced Oct 7, 2022
Closed
Closed
@inancgumus inancgumus added the events CDP or internal events related. label Oct 18, 2022
@inancgumus inancgumus changed the title The ordering of emitted events is sometimes lost which results in handlers consuming them out of order Deadlock when emitting and processing events Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ankur22 ankur22

Labels
bug Something isn't working events CDP or internal events related. next Might be eligible for the next planning (not guaranteed!)
Projects
None yet
Milestone
v0.6.0
Development

Successfully merging a pull request may close this issue.

Add a queue per handler to keep events in order grafana/xk6-browser
2 participants
@inancgumus @ankur22