Skip to content

Commit

Permalink
fix(@embark/core): fix memory leak when contract files are loaded
Browse files Browse the repository at this point in the history 
Prior to this commits `this.contractsFiles` kept growing at run time
for every file change that was detected by Embark.

The reason for that was a bug in a condition that determines whether
a subset of contract files needed to be added to memory or not.

In addition to that we kept pushing externally loaded contract files into
memory, even if they already existed in memory.
  • Loading branch information
@0x-r4bbit @iurimatias
0x-r4bbit authored and iurimatias committed Feb 8, 2019
1 parent 2e763f3 commit 40b9ac3
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 22 deletions.
57 changes: 38 additions & 19 deletions packages/embark/src/lib/core/config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ var Config = function(options) {
resolver = resolver || function(callback) {
callback(fs.readFileSync(filename).toString());
};
self.contractsFiles.push(new File({path: filename, type: Types.custom, resolver}));
self.contractsFiles.push(new File({path: filename, originalPath: filename, type: Types.custom, resolver}));
});

self.events.on('file-remove', (fileType, removedPath) => {
Expand Down Expand Up @@ -120,15 +120,16 @@ Config.prototype.reloadConfig = function() {
};

Config.prototype.loadContractFiles = function() {
const contracts = this.embarkConfig.contracts;
const newContractsFiles = this.loadFiles(contracts);
if (!this.contractFiles || newContractsFiles.length !== this.contractFiles.length || !deepEqual(newContractsFiles, this.contractFiles)) {
this.contractsFiles = this.contractsFiles.concat(newContractsFiles).filter((file, index, arr) => {
return !arr.some((file2, index2) => {
return file.path === file2.path && index < index2;
});
});
}
const loadedContractFiles = this.loadFiles(this.embarkConfig.contracts);
// `this.contractsFiles` could've been mutated at runtime using
// either `config:contractsFiles:add` event or through calls to
// `loadExternalContractsFiles()`, so we have to make sure we preserve
// those added files before we reset `this.contractsFiles`.
//
// We do that by determining the difference between `loadedContractFiles` and the ones
// already in memory in `this.contractsFiles`.
const addedContractFiles = this.contractsFiles.filter(existingFile => !loadedContractFiles.some(file => file.originalPath === existingFile.originalPath));
this.contractsFiles = loadedContractFiles.concat(addedContractFiles);
};

Config.prototype._updateBlockchainCors = function(){
Expand Down Expand Up @@ -358,20 +359,36 @@ Config.prototype.loadExternalContractsFiles = function() {
}
for (let contractName in contracts) {
let contract = contracts[contractName];

if (!contract.file) {
continue;
}

let externalContractFile = null;

if (contract.file.startsWith('http') || contract.file.startsWith('git') || contract.file.startsWith('ipfs') || contract.file.startsWith('bzz')) {
const fileObj = utils.getExternalContractUrl(contract.file,this.providerUrl);
const fileObj = utils.getExternalContractUrl(contract.file, this.providerUrl);
if (!fileObj) {
return this.logger.error(__("HTTP contract file not found") + ": " + contract.file);
}
const localFile = fileObj.filePath;
this.contractsFiles.push(new File({path: localFile, type: Types.http, basedir: '', externalUrl: fileObj.url, storageConfig: storageConfig}));
externalContractFile = new File({ path: fileObj.filePath, originalPath: fileObj.filePath, type: Types.http, basedir: '', externalUrl: fileObj.url, storageConfig });
} else if (fs.existsSync(contract.file)) {
this.contractsFiles.push(new File({path: contract.file, type: Types.dappFile, basedir: '', storageConfig: storageConfig}));
externalContractFile = new File({ path: contract.file, originalPath: contract.file, type: Types.dappFile, basedir: '', storageConfig });
} else if (fs.existsSync(path.join('./node_modules/', contract.file))) {
this.contractsFiles.push(new File({path: path.join('./node_modules/', contract.file), type: Types.dappFile, basedir: '', storageConfig: storageConfig}));
const completePath = path.join('./node_modules/', contract.file);
externalContractFile = new File({ path: completePath, originalPath: completePath, type: Types.dappFile, basedir: '', storageConfig });
}

if (externalContractFile) {
const index = this.contractsFiles.findIndex(contractFile => contractFile.originalPath === externalContractFile.originalPath);
// It's important that we only add `externalContractFile` if it doesn't exist already
// within `contractsFiles`, otherwise we keep adding duplicates in subsequent
// compilation routines creating a memory leak.
if (index > -1) {
this.contractsFiles[index] = externalContractFile;
} else {
this.contractsFiles.push(externalContractFile);
}
} else {
this.logger.error(__("contract file not found") + ": " + contract.file);
}
Expand Down Expand Up @@ -563,7 +580,7 @@ Config.prototype.loadFiles = function(files) {
return (file[0] === '$' || file.indexOf('.') >= 0);
}).filter(function(file) {
let basedir = findMatchingExpression(file, files);
readFiles.push(new File({path: file, type: Types.dappFile, basedir: basedir, storageConfig: storageConfig}));
readFiles.push(new File({path: file, originalPath: file, type: Types.dappFile, basedir: basedir, storageConfig: storageConfig}));
});

var filesFromPlugins = [];
Expand Down Expand Up @@ -597,9 +614,11 @@ Config.prototype.loadPluginContractFiles = function() {
contractsPlugins.forEach(function(plugin) {
plugin.contractsFiles.forEach(function(file) {
var filename = file.replace('./','');
self.contractsFiles.push(new File({path: filename, pluginPath: plugin.pluginPath, type: Types.custom, storageConfig: storageConfig, resolver: function(callback) {
callback(plugin.loadPluginFile(file));
}}));
self.contractsFiles.push(new File({ path: filename, originalPath: path.join(plugin.pluginPath, filename), pluginPath: plugin.pluginPath, type: Types.custom, storageConfig,
resolver: function(callback) {
callback(plugin.loadPluginFile(file));
}
}));
});
});
};
Expand Down
2 changes: 2 additions & 0 deletions packages/embark/src/lib/core/file.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export class File {
public storageConfig: any;
public providerUrl: string;
public importRemappings: ImportRemapping[] = [];
public originalPath: string;

constructor(options: any) {
this.type = options.type;
Expand All @@ -34,6 +35,7 @@ export class File {
this.pluginPath = options.pluginPath ? options.pluginPath : "";
this.storageConfig = options.storageConfig;
this.providerUrl = "";
this.originalPath = options.originalPath || "";

if (this.type === Types.http) {
const external = utils.getExternalContractUrl(options.externalUrl, this.providerUrl);
Expand Down
6 changes: 3 additions & 3 deletions packages/embark/src/lib/utils/solidity/remapImports.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,23 +65,23 @@ const buildNewFile = (file: File, importPath: string) => {
from = resolve(importPath);
to = fs.dappPath(".embark", "node_modules", importPath);
fs.copySync(from, to);
return new File({ path: to, type: Types.dappFile });
return new File({ path: to, type: Types.dappFile, originalPath: from });
}

// started with node_modules then further imports local paths in it's own repo/directory
if (isEmbarkNodeModule(file.path)) {
from = path.join(path.dirname(file.path.replace(".embark", ".")), importPath);
to = path.join(path.dirname(file.path), importPath);
fs.copySync(from, to);
return new File({ path: to, type: Types.dappFile });
return new File({ path: to, type: Types.dappFile, originalPath: from });
}

// local import, ie import "../path/to/contract" or "./path/to/contract"
from = path.join(path.dirname(file.path.replace(".embark", ".")), importPath);
to = path.join(path.dirname(file.path), importPath);

fs.copySync(from, to);
return new File({ path: to, type: Types.dappFile });
return new File({ path: to, type: Types.dappFile, originalPath: from });
};

const rescursivelyFindRemapImports = async (file: File, filesProcessed: string[] = []) => {
Expand Down
3 changes: 3 additions & 0 deletions packages/embark/src/test/config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ describe('embark.Config', function () {
"type": "http",
"externalUrl": "https://raw.githubusercontent.com/embark-framework/embark/master/test_dapps/packages/test_app/app/contracts/simple_storage.sol",
"path": ".embark/contracts/embark-framework/embark/master/test_dapps/packages/test_app/app/contracts/simple_storage.sol",
"originalPath": ".embark/contracts/embark-framework/embark/master/test_dapps/packages/test_app/app/contracts/simple_storage.sol",
"pluginPath": '',
"basedir": "",
"importRemappings": [],
Expand All @@ -203,6 +204,7 @@ describe('embark.Config', function () {
"type": "http",
"externalUrl": "https://raw.githubusercontent.com/status-im/contracts/master/contracts/identity/ERC725.sol",
"path": ".embark/contracts/status-im/contracts/master/contracts/identity/ERC725.sol",
"originalPath": ".embark/contracts/status-im/contracts/master/contracts/identity/ERC725.sol",
"pluginPath": '',
"basedir": "",
"importRemappings": [],
Expand All @@ -213,6 +215,7 @@ describe('embark.Config', function () {
{
"externalUrl": "https://swarm-gateways.net/bzz:/1ffe993abc835f480f688d07ad75ad1dbdbd1ddb368a08b7ed4d3e400771dd63",
"path": ".embark/contracts/bzz:/1ffe993abc835f480f688d07ad75ad1dbdbd1ddb368a08b7ed4d3e400771dd63",
"originalPath": ".embark/contracts/bzz:/1ffe993abc835f480f688d07ad75ad1dbdbd1ddb368a08b7ed4d3e400771dd63",
"type": "http",
"pluginPath": '',
"basedir": "",
Expand Down

0 comments on commit 40b9ac3

Please sign in to comment.