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.

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 spawning actors with duplicate id bug #164

Merged
merged 6 commits into from
Aug 22, 2024

Conversation

troygilman0
Copy link
Contributor

This PR addresses this issue #163.

  • Registry.add() now returns an error if the process exists.
  • Instead of running through the actor startup sequence for the duplicate process, we will just return the PID as it should be a valid PID for the original process.
  • Added test case to ensure only one process is started.

@@ -223,7 +223,7 @@ func TestSpawn(t *testing.T) {
wg.Wait()
}

func TestSpawnDuplicateId(t *testing.T) {
func TestSpawnDuplicateKind(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed this existing test case to be more accurate

@tprifti
Copy link
Contributor

tprifti commented Aug 19, 2024

If you check Registry, it's not possible to add 2 actors with same ID. An event is broadcasted and you can subscribe into that in case you want to have a specific logic.
For reference: https://github.com/anthdm/hollywood/blob/master/actor/registry.go#L60-L70

@troygilman0
Copy link
Contributor Author

troygilman0 commented Aug 19, 2024

If you check Registry, it's not possible to add 2 actors with same ID. An event is broadcasted and you can subscribe into that in case you want to have a specific logic. For reference: https://github.com/anthdm/hollywood/blob/master/actor/registry.go#L60-L70

I understand that there is a DuplicateIdEvent that is published in this scenario, but the issue is that the startup sequence (actor.Initialized and actor.Started events) is still triggered for the duplicate actor. This can be seen in the issue I raised #163. In my opinion, this is unexpected behavior that can cause bugs.

@tprifti
Copy link
Contributor

tprifti commented Aug 19, 2024

If you check Registry, it's not possible to add 2 actors with same ID. An event is broadcasted and you can subscribe into that in case you want to have a specific logic. For reference: https://github.com/anthdm/hollywood/blob/master/actor/registry.go#L60-L70

I understand that there is a DuplicateIdEvent that is published in this scenario, but the issue is that the startup sequence (actor.Initialized and actor.Started events) is still triggered for the duplicate actor. This can be seen in the issue I raised #163. In my opinion, this is unexpected behavior that can cause bugs.

I will investigate it further and see what is causing the issue. Normally, the registry should not do anything when the process exists, thus no duplicate events should be present

@anthdm
Copy link
Owner

anthdm commented Aug 21, 2024

@troygilman0 @tprifti After some investigation I think we need to prevent engine.SpawnProc from calling Start() and move this to the bottom of the registry.Add function.

func (e *Engine) SpawnProc(p Processer) *PID {
	e.Registry.add(p)
	// p.Start() remove this!
	return p.PID()
}
func (r *Registry) add(proc Processer) {
	r.mu.Lock()
	id := proc.PID().ID
	if _, ok := r.lookup[id]; ok {
		r.mu.Unlock()
		r.engine.BroadcastEvent(ActorDuplicateIdEvent{PID: proc.PID()})
		return
	}
	r.lookup[id] = proc
	r.mu.Unlock()

	proc.Start() // Add this
}

@anthdm anthdm merged commit d199384 into anthdm:master Aug 22, 2024
1 check passed
@anthdm
Copy link
Owner

anthdm commented Aug 22, 2024

Looks good to go. Thanks for all the effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants