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

Closed
asselstine opened this issue Jan 25, 2019 · 12 comments
Closed
Assignees
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published.

Comments

@asselstine
Copy link

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 Under investigation and may be a bug. label Jan 25, 2019
@ricmoo
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 Verified to be an issue. on-deck This Enhancement or Bug is currently being worked on. and removed investigate Under investigation and may be a bug. labels Jan 25, 2019
@ricmoo
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.

@asselstine
Copy link
Author

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
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
Copy link
Author

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
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
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
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
Copy link
Author

Excellent! Thank you

ricmoo added a commit that referenced this issue May 23, 2019
@krzkaczor
Copy link

@ricmoo should this be closed?

@ricmoo
Copy link
Member

ricmoo commented Apr 4, 2020

@krzkaczor Yes, I'll close this now. It is still an issue in v4, but seems like a dangerous and involved change for v4. It works fine in v5, and this required a fairly substantial re-write of how events work, which is how I know it is risky for v4.

Thanks!

@ricmoo ricmoo closed this as completed Apr 4, 2020
@asselstine
Copy link
Author

All the more reason to upgrade to v5! Thank you.

@ricmoo ricmoo removed the on-deck This Enhancement or Bug is currently being worked on. label Apr 22, 2020
michaeltout pushed a commit to michaeltout/ethers.js that referenced this issue Aug 23, 2020
@ricmoo ricmoo added the fixed/complete This Bug is fixed or Enhancement is complete and published. label Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published.
Projects
None yet
Development

No branches or pull requests

3 participants