Contract leak fix

[chancehudson]

Description

Removes setProvider variable inside Contract constructor. This solves the memory leak when creating contract objects using new web3.eth.Contract(...). This fix is tested here.

Fixes #3042

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

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 ran npm run test:cov and my test cases cover all the lines and branches of the added code.
• I ran npm run build and tested dist/web3.min.js in a browser.
• I have tested my code on the live network.
• I have checked the Deploy Preview and it looks correct.
• I have updated the CHANGELOG.md file in the root folder.

[coveralls]

Pull Request Test Coverage Report for Build 511416922

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 385 unchanged lines in 7 files lost coverage.
• Overall coverage decreased (-2.5%) to 73.759%

---

Files with Coverage Reduction	New Missed Lines	%
packages/web3-core-requestmanager/src/jsonrpc.js	1	88.0%
packages/web3-core-helpers/src/formatters.js	24	79.42%
packages/web3-core-helpers/src/errors.js	31	4.41%
packages/web3-utils/src/soliditySha3.js	55	5.56%
packages/web3-utils/src/index.js	60	30.0%
packages/web3-utils/src/utils.js	84	13.24%
packages/web3-eth-accounts/src/index.js	130	31.65%

Totals
Change from base Build 510647036:	-2.5%
Covered Lines:	3292
Relevant Lines:	4223

---

💛 - Coveralls

[GregTheGreek]

@koraykoska lets take a look at this. Relevant for the refactor

[GregTheGreek]

Just want to let you know I am reviewing this, but I'm taking some extra care to better understand if there are any odd implications by removing the var setProvider...

[GregTheGreek]

@jchancehud Can you explain how you discovered this was the solution?

[chancehudson]

@GregTheGreek Sure, I initially thought the reference to self was causing the leak so I used a WeakRef to store the reference. This solved the memory leak but broke the expected behavior of web3 setProvider updating all contracts created from it.

So I looked at it more and realized each Contract creation was storing a new reference to setProvider that already existed in the scope above. The function of the variable was the same, to keep a reference to the original setProvider function as the property itself was overwritten. So I suspected the original setProvider could be re-used in each contract creation and I seemed to be correct.

[GregTheGreek]

@jchancehud Thanks!

We're still investigating the scoping that this PR affects (inheriting some old code). Hopefully if all things are good we can get this in!

[chancehudson]

Great, just ping me if/when it's ready and I'll do a final rebase.

[transmissions11]

just ran into this... this should be a top priority. initializing a lot of contracts will eat up gigbabytes

[chancehudson]

Yea, I'd like an update on this as well. @GregTheGreek is there something specific holding this up?

[kevin-leptons]

Hi everybody, this should be 1st priority to work with.
I just show here to push up 🥇.

[jun0tpyrc]

Also running into this

[MrPeanutz]

I'm experiencing huge memory leaks overtime in my dapp, probably because of this, any ETA on PR merge?

[bgmulinari]

Hello. This issue is also affecting me and causing me a lot of headaches. Why is it taking so long for this fix to be merged?

[chancehudson]

Bump, it's been a cool 5 months on this.

[transmissions11]

merge pls lmao

[github-actions]

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[tarzansarka]

bump

[mirror-ru]

Bump

[cpecorari]

Hi, ended up on this thread after huge memory leak in our node App, seems like it's coming from Web3's Contract setprovider... so bump !

[addo47]

Bump!

[haydendaly]

Having this issue as well, please review!

[ruffolom]

Really need this reviewed! Thanks

[jdevcs]

Related with #4580

[nazarhussain]

As explained in the #4580 (comment) this PR is introducing a breaking change to fix the the problem (which actually is a desired behavior). To update current provider for all instances of the contracts created, we have to keep reference of all objects. That is a memory leak in a sense but can't be fixed without introducing breaking changed in an expected behavior. This PR fixes the leak, but introduces breaking change in the behavior of updating current provider of all contract instances.

As explained in #4580 (comment) we will look into some workaround for it in 4.x. For now would be really nice to open a PR for updating 1.x documentation by mentioning this expected behavior.

[janeklb]

@nazarhussain could you document (even here, in a github comment) the desired behaviour that is broken by this change? We're facing this issue and would like to find a workaround for 1.x - we are considering forking the repo or monkeypatching the code to address this temporarily while we wait for 4.x).

Thank you :)

[nazarhussain]

@janeklb As of the 1.x the behavior is as follows.

If web3.eth.Contract.setProvider is invoked it should update the provider for **all the contract instances ever created with new web3.eth.Contract in the life cycle of web3 object.

To have this behavior, we have to keep the reference of all contract instances created so that when web3.eth.Contract.setProvider is invoked it should update provider of all contract instances.

const c1 = new web3.eth.Contract([...abi], address1);
const c2 = new web3.eth.Contract([...abi], address1);

If we call any of following:

web3.eth.Contract.setProvider

or

c1.setProvider 

it will update the provider for both c1 and c2. So why we have to keep reference of c1 and c2 inside web3 object. It's not a memory leak but desired behavior.

In your application if you feel there is a memory leak because of this behavior, then you must improve the logic of your initiating the contract in your code. The contract objects should not be created aggressively, rather created once on a global scope and then used else where.

For the 4.x compatibility with the 1.x we kept the behavior as it is for now. After 4.x alpha released we may think of changing this behavior within a major release.

[janeklb]

Thanks @nazarhussain!

In our case we do not call setProvider and so it feels like this functionality can be removed. I think I've managed to achieve this with the following monkeypatch

const BaseContract = require('web3-eth-contract');

module.exports = {
  patchWeb3: function (web3) {
    // create a very bare-bones contract class by extending BaseContract and not plugging into setProvider at all basically replacing all of
    // https://github.com/ChainSafe/web3.js/blob/b32555cfeedde128c657dabbba201102f691f955/packages/web3-eth/src/index.js#L340-L363
    class Contract extends BaseContract {}

    // and then just update all the references as web3.eth already does
    // https://github.com/ChainSafe/web3.js/blob/b32555cfeedde128c657dabbba201102f691f955/packages/web3-eth/src/index.js#L365-L377
    web3.eth.Contract = Contract;
    Contract.defaultAccount = web3.eth.defaultAccount;
    Contract.defaultBlock = web3.eth.defaultBlock;
    Contract.transactionBlockTimeout = web3.eth.transactionBlockTimeout;
    Contract.transactionConfirmationBlocks = web3.eth.transactionConfirmationBlocks;
    Contract.transactionPollingTimeout = web3.eth.transactionPollingTimeout;
    Contract.transactionPollingInterval = web3.eth.transactionPollingInterval;
    Contract.blockHeaderTimeout = web3.eth.blockHeaderTimeout;
    Contract.handleRevert = web3.eth.handleRevert;
    Contract._requestManager = web3.eth._requestManager;
    Contract._ethAccounts = web3.eth.accounts;
    Contract.currentProvider = web3.eth._requestManager.provider;

    return web3;
  },
};

then just call patchWeb3 on your Web3 objects

const web3 = patchWeb3(new Web3("..."));

Given that we do not explicitly call Contract#setProvider in our code, I think this is safe. Do you have any thoughts about this approach?

[janeklb]

In your application if you feel there is a memory leak because of this behavior, then you must improve the logic of your initiating the contract in your code. The contract objects should not be created aggressively, rather created once on a global scope and then used else where.

FWIW this isn't really possible for us -- we deal with an unbound number of addresses so we can't really re-use Contracts (afaiu at least.. you can't/shouldn't update the address on a contract after it;s been created)

[nazarhussain]

@janeklb That seems good to me. I personally are not the fan of this feature, but as of policy for not making any breaking changes in 1.x we can't remove it.

In 4.x I believe we can get rid of this behavior completely. User don't need to change the provider of all contracts often and if they need they should manage instances of contracts themselves the way their application architecture is.

Due to 4.x compatibility with 1.x we are keeping this behavior in 4.x-alpha release, but it would be very easy to remove after 4.x release as we will have continuous cycle of introducing breaking changes to improve the wbe3.js.

[krboktv]

Had the same issue with memory leak. It was necessary to create many contract instances during the lifetime of the application. Few lines below helped me to solve this issue for v1.7.4:

const Contract = require('web3-eth-contract')
const Web3 = require('web3')

const contractAddress = '0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2'
const ABI = [{"constant":true,"inputs":[{"name":"","type":"address"}],"name":"balanceOf","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"}]
const nodeUrl = '...'
const web3 = new Web3(nodeUrl)

Contract.setProvider(web3.currentProvider)

const contract = new Contract(ABI, contractAddress)

[dev-cz]

Had the same issue with memory leak. It was necessary to create many contract instances during the lifetime of the application. Few lines below helped me to solve this issue for v1.7.4:

const Contract = require('web3-eth-contract')
const Web3 = require('web3')

const contractAddress = '0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2'
const ABI = [{"constant":true,"inputs":[{"name":"","type":"address"}],"name":"balanceOf","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"}]
const nodeUrl = '...'
const web3 = new Web3(nodeUrl)

Contract.setProvider(web3.currentProvider)

const contract = new Contract(ABI, contractAddress)

Can you expand a bit on how this works? I have an express served that leverages web3 and have about 7/8 contracts created within one function.