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

Events matching a filter are fired by the square of the number of filters #404

Open
asselstine opened this Issue Jan 25, 2019 · 9 comments

Comments

Projects
None yet
2 participants
@asselstine
Copy link

asselstine commented Jan 25, 2019

Ok, found some weird behaviour that may need an explanation, or is a bug.

When I listen for an event in multiple places, i.e.:

const contract = new ethers.Contract(...etc, ethersProvider)

var filter = contract.filters.TestEvent()

ethersProvider.on(filter, console.log) // listen in one place

ethersProvider.on(filter, console.log) // listen in another place

When the smart contract emits a TestEvent, the above code prints out the event four times.

If I added a third ethersProvider.on(filter, console.log) then it prints out the event nine times.

I think I have an explanation:

  1. Looking at the BaseProvider class, it appears that the filters are stored in a _events array using the _addEventListener() function.

  2. Events are broadcast using a polling function. When a new block comes in, it loops through all of the _events to see what needs to be emitted from the block.

For each log that matches the _event, it is emitted using the emit() function

  1. Finally, the emit() function dispatches the log to every _event that was listening to it.

Now, this would explain the (event listeners)^2 results I was getting above, because:

  1. poll(): For each listener, find the block logs that match the listener and call emit()
  2. emit(): Receive log, find each listener that is listening to the log and dispatch it

Thoughts?

@ricmoo ricmoo self-assigned this Jan 25, 2019

@ricmoo ricmoo added the investigate label Jan 25, 2019

@ricmoo

This comment has been minimized.

Copy link
Member

ricmoo commented Jan 25, 2019

Weird and quite possible; if so it is a bug. I will investigate, but I think your explanation would make sense.

@ricmoo ricmoo added bug on-deck and removed investigate labels Jan 25, 2019

@ricmoo

This comment has been minimized.

Copy link
Member

ricmoo commented Jan 25, 2019

I've confirmed this is a bug and have a consistent script to reproduce it. I'm working on it now, but have to jet soon, so it may not be fixed until tomorrow.

ricmoo added a commit that referenced this issue Jan 26, 2019

@asselstine

This comment has been minimized.

Copy link
Author

asselstine commented Jan 26, 2019

Fantastic! I'm glad it's getting fixed. We actually switched to Ethers from web3 1.0 because it was having a similar issue.

Thanks for being so on top of it!

@ricmoo

This comment has been minimized.

Copy link
Member

ricmoo commented Jan 26, 2019

I have a fix in place, if you want to pull the latest code you can try it locally. Travis CI takes about 35 minutes to run all the test cases, and I will probably sleep on this fix so I can mull it over, but seems like this is the right thing to do. :)

@asselstine

This comment has been minimized.

Copy link
Author

asselstine commented Jan 26, 2019

Tested the latest code- the fix works for provider.on() but fails for contract.on()

My test was:

const filter = contract.filters.Test()

provider.on(filter, console.log)
provider.on(filter, console.log)

I then transacted with Ganache for one event and there was two console.logs. Correct!

However, this test:

const filter = contract.filters.Test()

contract.on(filter, console.log)
contract.on(filter, console.log)

The above test still prints four console.logs, so for some reason the contract specific one is still not working.

@ricmoo

This comment has been minimized.

Copy link
Member

ricmoo commented Jan 26, 2019

Awesome! Glad I held off publishing. :)

I’ll investigate further tonight... May be similar code in the Contract... it was hard to generalize that functionality...

@ricmoo

This comment has been minimized.

Copy link
Member

ricmoo commented Jan 27, 2019

I've identified the issue; very similar, and it will require some non-trivial changes. I will get to this soon, but it will require much more testing and thinking things through, so it might be a bit longer before it is ready to be published. :s

@ricmoo

This comment has been minimized.

Copy link
Member

ricmoo commented Feb 21, 2019

Just following up to make sure you know I haven't forgotten about this. It is next on my plate. :)

@asselstine

This comment has been minimized.

Copy link
Author

asselstine commented Feb 21, 2019

Excellent! Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.