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

Anonymous functions are incompatible with the event emitter #15

Open
tul opened this issue Feb 6, 2018 · 1 comment
Open

Anonymous functions are incompatible with the event emitter #15

tul opened this issue Feb 6, 2018 · 1 comment

Comments

@tul
Copy link

tul commented Feb 6, 2018

If you create multiple closures using the same anonymous function, they all have the same pointer address because of how Go works. If you add these anonymous functions to the event emitter, removal of one of the listeners results in the removal of all of them.

package main

import (
"fmt"
"github.com/chuckpreslar/emission"
)

type action func()

func getCallback(myNumber uint32) action {
	fn := func() {
		fmt.Printf("number is %d\n", myNumber)
	}
	return fn
}

func main() {
	five := getCallback(5)
	six := getCallback(6)

	five()
	six()

	fmt.Printf("five addr %p six addr %p\n", five, six)  // <-- same address printed

	emitter := emission.NewEmitter()
	emitter.On("test",five)
	emitter.On("test",six)
	emitter.RemoveListener("test",five) // <-- removes both five and six
	emitter.EmitSync("test") // <-- does nothing (no listeners)
}

In this example, local vars five and six hold unique closures, which if executed will print different messages; however they have the same address. As the emitter tracks listeners by address, if we add both of these to the emitter on the same event, then remove one, it will remove both.

tul added a commit to tul/emission that referenced this issue Apr 25, 2018
required changing the interface, you can no longer remove listeners by
function reference, but have to use an explicit `handle` which is returned
when your listener is added
@tul
Copy link
Author

tul commented Apr 25, 2018

I've proposed a fix in pull request #16, but as it breaks the existing interface it probably isn't something you'll want to merge - see what you think. Thanks!

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

No branches or pull requests

1 participant