Make maxListenersExceeded warning configurable

[cgewecke]

Description

Adds a configuration setting: maxListenersWarningThreshold which can be used to silence Node warnings about the number of listeners being attached to provider when setProvider is called.

Raises the current threshold from 10 to 100.

At the moment, a new RequestManager is created and 4 listeners are attached when:

• setProvider is called on a contract (something that typically happens on initialization and is bounded by the number of contracts in a dapp).
• the provider supports sockets (e.g. ws | ipc)

The number of listeners will generally be proportionate to the number of contract abstractions a dapp or script interacts with.

Addresses #1648

Type of change

• Feature

Checklist:

• I have selected the correct base branch.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• Any dependent changes have been merged and published in downstream modules.
• I ran npm run dtslint with success and extended the tests and types if necessary.
• I ran npm run test:unit with success.
• I have executed npm run test:cov and my test cases do cover all lines and branches of the added code.
• I ran npm run build-all and tested the resulting file/'s from dist folder in a browser.
• I have updated the CHANGELOG.md file in the root folder.
• I have tested my code on the live network.

[wbt]

FYI: The key use case here is probably in calling setProvider on several different contracts instead of calling it on the same contract repeatedly. It is possible for a fix to address the test for the latter without addressing the former, e.g. by having the contract check to see if the provider has already been set to what it's being set to again, but that fix wouldn't address the observable issue for the practical use case.

[cgewecke]

The key use case here is probably in calling setProvider on several different contracts instead of calling it on the same contract repeatedly.

Yes, I tried this as well. That's probably the most common way people would hit it. It's possible the right thing to do here is just make the warning threshold configurable - need to investigate that.

[nivida]

This should be fixed with the provider improvements PR I've created or there is still a small missing case but I should have fixed it. The listener will/should not get re-added and instead it should "forward" the already existing and newly created RequestManager instance. It will also detect the same provider if I'm right. (just my two cents) @cgewecke :-)

[wbt]

This should be fixed with the provider improvements PR I've created or there is still a small missing case but I should have fixed it.

@nivida by "provider improvements PR" are you referring to #3190? I tried building & using the 1.x branch with that PR merged in, and still observed the MaxListenersExceededWarning. Therefore, I don't think #3190 fully fixes it, though I was hopeful. Thanks for your work on that!

[wbt]

Yes, I tried this as well. That's probably the most common way people would hit it. It's possible the right thing to do here is just make the warning threshold configurable - need to investigate that.

I also looked in to changing the warning threshold, but all the sources I found advised strongly against doing so, as the warning helps find memory leaks. The primary objective of the issue is to not have a memory leak; it isn't just to silence a console warning.

For me, the ultimate test on passing this Issue will be when one can have a Dapp with several smart contracts that each have a provider set, and there is NO custom configuration of a warning threshold, and no warning, because there is no memory leak to warn about. I am not sure how easy that is to code into unit testing or how much emphasis you put on unit testing in this project. The test added in this draft PR is just one small step in the right direction: it should pass but currently doesn't (so it's helpful to have in the suite), but could be made to pass without the real underlying issue being addressed (so it's insufficient if trying to be rigorous about testing). However, if this is an issue of limited effort-hours on a volunteer project being prioritized toward actual fixes over unit tests, that is quite understandable, as long as the actual underlying objective remains clear (hopefully, comments like these can help).

[cgewecke]

@wbt Agree this needs many more tests and further investigation since we shouldn't be attaching unnecessary listeners. Just getting started here...

For additional background - allowing the user to configure the threshold is (a little) like letting them make an explicit memory allocation. It's not uncommon for people to set the allowed emitters higher where necessary (see eslint 524).

[coveralls]

Coverage increased (+0.05%) to 89.833% when pulling fa6435e on issue/1942 into 242bfd0 on 1.x.

[cgewecke]

Spent an (inordinate) amount of time trying to fix this the "right" way but failed.

At the moment, when setProvider is called on a contract, a new RequestManager is created which handles its event subscriptions via callbacks registered with a shared provider.

For tools like Truffle that call setProvider on contract abstractions as part of their regular test setup flow, the "leak" is proportionate to the number of objects being managed.

This could be rationalized in a refactor but tbh I'm not comfortable making the kind of intervention that would be required here. It feels very risky.

There would be a stronger case for doing this if people reported that their long-running server side scripts are running OOM or people's pages are consuming huge amounts of memory. But I don't see open issues like that here.

[wbt]

There would be a stronger case for doing this if people reported that their long-running server side scripts are running OOM or people's pages are consuming huge amounts of memory. But I don't see open issues like that here.

I am seeing signs of that, but haven't gotten them conclusively boiled down to a specific issue in web3, succinct enough for posting here. Issues in such dynamic environments are hard to reproduce reliably with original code, let alone getting the issue reproducing with simplified code that is more sharable and diagnostic.

In summary, I don't think we should assume that a lack of such reports actually indicate a lack of such issues.

[cgewecke]

In summary, I don't think we should assume that a lack of such reports actually indicate a lack of such issues.

@wbt Agree. However in this case 4 listeners are attached when:

• the provider supports sockets
• setProvider is called on a contract (something that typically happens on initialization and is bounded by the number of contracts in a dapp).

A more conventional and worrying leak would be: attaching new listeners for every data request. There it's obvious that long-running scripts would have a persistently growing mem. usage.

Can you think of a real-world context or design pattern where setProvider's current behavior would tend towards OOM?

[ryanio]

Lgtm! I agree with @wbt, @cgewecke et al. that the code around setProvider is overdue for a refactor, but on limited time I think this is a good solution for now.