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

Contract.removeAllListeners issue #391

Closed
marcelomorgado opened this issue Jan 15, 2019 · 16 comments
Closed

Contract.removeAllListeners issue #391

marcelomorgado opened this issue Jan 15, 2019 · 16 comments
Assignees
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published.

Comments

@marcelomorgado
Copy link

Seems that removeAllListener method isn't removing listeners properly.
I've written the test case below to simulate the expected behavior and how it seems wrong.
Tested on ethers.js 4.0.12 and 4.0.20 versions.

  it("ethers.js removeAllListeners bug - listeners remain registered", async () => {
    const eventFakeFn = sinon.fake();

    const sentTx = await contract.setValue("hello world");

    let eventListener;

    await new Promise(function(resolve, reject) {
      eventListener = () => {
        eventFakeFn();
        resolve();
      };
      contract.on("ValueChanged", eventListener);
    });

    // Not working
    contract.removeAllListeners("ValueChanged");
    // Working
    // contract.removeListener("ValueChanged", eventListener);

    expect(eventFakeFn.called).to.be.true;
    expect(contract.listenerCount("ValueChanged")).to.equal(0);
    expect(contract.provider._events).to.be.empty;
  });

Thanks

@ricmoo ricmoo added investigate Under investigation and may be a bug. on-deck This Enhancement or Bug is currently being worked on. labels Jan 15, 2019
@ricmoo ricmoo self-assigned this Jan 15, 2019
@ricmoo
Copy link
Member

ricmoo commented Jan 15, 2019

Ack! Looking at the contract code, I believe you are correct; I do not pass the de-registration onto the provider, just remove it from the member events list. I will look into this shortly.

Thanks! :)

@ricmoo
Copy link
Member

ricmoo commented Jan 15, 2019

Ok. I have a simple example running. Just to confirm, your problem is not that events are being triggered after being removed. The problem is that the listener lingers, and therefore doesn't shut down the process, correct?

@marcelomorgado
Copy link
Author

Yes, the process still runs and contract.provider._events remains with an event even after removal.

@marcelomorgado
Copy link
Author

I've not checked if the event is being triggered after remove but seems that didn't.

@ricmoo ricmoo added bug Verified to be an issue. and removed investigate Under investigation and may be a bug. labels Jan 15, 2019
@ricmoo
Copy link
Member

ricmoo commented Jan 15, 2019

Can you try out 4.0.21 and see if that solves your problem?

Thanks! :)

@marcelomorgado
Copy link
Author

It's working now.
Thank you!

@ricmoo ricmoo removed the on-deck This Enhancement or Bug is currently being worked on. label Jan 16, 2019
@ricmoo
Copy link
Member

ricmoo commented Jan 16, 2019

Awesome!!

Thanks! :)

@ricmoo ricmoo closed this as completed Jan 16, 2019
@ricmoo ricmoo added the fixed/complete This Bug is fixed or Enhancement is complete and published. label Jan 11, 2020
@JefferyHus
Copy link

This is still a problem using the latest version 5.0.32 I am unable to stop/remove ongoing listeners

@ricmoo
Copy link
Member

ricmoo commented May 30, 2021

@JefferyHus the most recent version is 5.2.0. Can you try that and let me know? I don’t think anything regarding this should have changed though…

If it is still a problem, can you please include some minimum code that reproduces it?

@JefferyHus
Copy link

JefferyHus commented Jun 1, 2021

@ricmoo

factory = new ethers.Contract(
  addresses.factory,
  [
    'event PairCreated(address indexed token0, address indexed token1, address pair, uint)'
  ],
  account,
);

factory.on('PairCreated', async (token0, token1, pairAddress) => {
  // do things here
 // if done
 factory.off('PairCreated'); // this fails, I even did this factory.removeAllListeners(); and I can still see the event registred
});

This is not working at all

@zemse
Copy link
Collaborator

zemse commented Jun 8, 2021

@JefferyHus if you want to execute an event once, you can do factory.once('PairCreated', callback).

Also in your example, can you confirm if the condition in your if statement is true? so that the flow actually goes in and factory.off is done

@JefferyHus
Copy link

My conditions do execute. I don't want to run it once but I should keep it running till I find the proper pair.

@ricmoo
Copy link
Member

ricmoo commented Jun 8, 2021

I tried this last week and did find there might possibly be a bug. I will test this again tomorrow. Are you using an address or ens name for the contract address?

@sayertindall
Copy link

running into this issue right now also... any updates?

@Quix44
Copy link

Quix44 commented May 8, 2022

still bugged in 5.6.5 even!

@omkar-bestdeveloperBTZ
Copy link

@JefferyHus any updates?

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

7 participants