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

feature(@embark/core): IPC adapts when run in Docker Linux on Win #1202

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions src/lib/core/env.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
/* global __dirname module process require */

const {execSync} = require('child_process');
const {delimiter} = require('path');
const {joinPath} = require('../utils/utils.js');
const {delimiter, join} = require('path');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of extracting these off of path. delimiter and join are concepts used in a lot more ways than just path, so it'll get a bit confusing down below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the usage context (args and neighboring variables/literals always have something do to w/ paths), I thought it wasn't too confusing, but willing to reconsider.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can just do const {delimiter as pathDelimiter, join as pathJoin} = require('path'); if we want them to be more clear


function anchoredValue(anchor, value) {
if (!arguments.length) {
Expand Down Expand Up @@ -39,11 +38,11 @@ const DEFAULT_CMD_HISTORY_SIZE = 20;
anchoredValue(CMD_HISTORY_SIZE, DEFAULT_CMD_HISTORY_SIZE);

const DIAGRAM_PATH = 'DIAGRAM_PATH';
const DEFAULT_DIAGRAM_PATH = joinPath(anchoredValue(DAPP_PATH), 'diagram.svg');
const DEFAULT_DIAGRAM_PATH = join(anchoredValue(DAPP_PATH), 'diagram.svg');
anchoredValue(DIAGRAM_PATH, DEFAULT_DIAGRAM_PATH);

const EMBARK_PATH = 'EMBARK_PATH';
const DEFAULT_EMBARK_PATH = joinPath(__dirname, '../../..');
const DEFAULT_EMBARK_PATH = join(__dirname, '../../..');
anchoredValue(EMBARK_PATH, DEFAULT_EMBARK_PATH);

const PKG_PATH = 'PKG_PATH';
Expand All @@ -52,7 +51,7 @@ anchoredValue(PKG_PATH, DEFAULT_PKG_PATH);

let YARN_GLOBAL_PATH;
try {
YARN_GLOBAL_PATH = joinPath(
YARN_GLOBAL_PATH = join(
execSync('yarn global dir', {stdio: ['pipe', 'pipe', 'ignore']})
.toString()
.trim(),
Expand All @@ -65,7 +64,7 @@ try {
const NODE_PATH = 'NODE_PATH';
// NOTE: setting NODE_PATH at runtime won't effect lookup behavior in the
// current process, but will take effect in child processes
process.env[NODE_PATH] = joinPath(anchoredValue(EMBARK_PATH), 'node_modules') +
process.env[NODE_PATH] = join(anchoredValue(EMBARK_PATH), 'node_modules') +
(YARN_GLOBAL_PATH ? delimiter : '') +
(YARN_GLOBAL_PATH || '') +
(process.env[NODE_PATH] ? delimiter : '') +
Expand Down
25 changes: 22 additions & 3 deletions src/lib/core/ipc.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,36 @@
const fs = require('./fs.js');
const ipc = require('node-ipc');
const {isDappMountedFromWindowsDockerHost} = require('../utils/host');
const os = require('os');
const {parse, stringify} = require('flatted/cjs');
const utils = require('../utils/utils.js');

class IPC {

constructor(options) {
this.logger = options.logger;
this.socketPath = options.socketPath || fs.dappPath(".embark/embark.ipc");
this.socketPath = options.socketPath || IPC.ipcPath('embark.ipc');
this.ipcRole = options.ipcRole;
ipc.config.silent = true;
this.connected = false;
}

static ipcPath(basename, usePipePathOnWindows = false) {
if (!(basename && typeof basename === 'string')) {
throw new TypeError('first argument must be a non-empty string');
}
let ipcDir;
if (process.platform === 'win32' && usePipePathOnWindows) {
return `\\\\.\\pipe\\${basename}`;
} else if (isDappMountedFromWindowsDockerHost) {
ipcDir = utils.joinPath(
os.tmpdir(), `embark-${utils.sha512(fs.dappPath()).slice(0, 8)}`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost exactly what I was thinking! I was thinking:

embark-<dapp_dir>-<dapp_full_path_sha512.substring(0,5)>

So, as an example:

Name Full path Path
embark_demo /Users/andremedeiros/src/_sandbox/embark_demo embark-embark_demo-f47bb
embark_demo /tmp/embark_demo embark-embark_demo-74a67
test_app /Users/andremedeiros/src/github.com/embark-framework/embark/test_apps/test_app embark-test_app-17efd

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea to put the original path itself into the directory name. I worry that things might get weird if spaces were in the dirname, though maybe not a problem in practice.

);
} else {
ipcDir = fs.dappPath('.embark');
}
return utils.joinPath(ipcDir, basename);
}

connect(done) {
const self = this;
function connecting(_socket) {
Expand Down Expand Up @@ -39,7 +58,7 @@ class IPC {
}

serve() {
fs.mkdirpSync(fs.dappPath(".embark"));
fs.mkdirpSync(utils.dirname(this.socketPath));
ipc.serve(this.socketPath, () => {});
ipc.server.start();

Expand Down
1 change: 0 additions & 1 deletion src/lib/core/processes/processManager.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

const ProcessState = {
Unstarted: 'unstarted',
Starting: 'starting',
Expand Down
5 changes: 4 additions & 1 deletion src/lib/modules/blockchain_process/gethClient.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const async = require('async');
const GethMiner = require('./miner');
const IPC = require('../../core/ipc');
const semver = require('semver');
const constants = require('../../constants');

Expand Down Expand Up @@ -50,7 +51,7 @@ class GethClient {
needKeepAlive() {
// TODO: check version also (geth version < 1.8.15)
if (this.isDev) {
// Trigger regular txs due to a bug in geth (< 1.8.15) and stuck transactions in --dev mode.
// Trigger regular txs due to a bug in geth (< 1.8.15) and stuck transactions in --dev mode.
return true;
}
return false;
Expand Down Expand Up @@ -78,6 +79,8 @@ class GethClient {
cmd.push("--verbosity=" + config.verbosity);
}

cmd.push(`--ipcpath=${IPC.ipcPath('geth.ipc', true)}`);

return cmd;
}

Expand Down
10 changes: 2 additions & 8 deletions src/lib/modules/blockchain_process/miner.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const async = require('async');
const IPC = require('../../core/ipc');
const NetcatClient = require('netcat/client');

//Constants
Expand Down Expand Up @@ -43,14 +44,7 @@ class GethMiner {
}
}

const isWin = process.platform === "win32";

let ipcPath;
if (isWin) {
ipcPath = '\\\\.\\pipe\\geth.ipc';
} else {
ipcPath = this.datadir + '/geth.ipc';
}
const ipcPath = IPC.ipcPath('geth.ipc', true);

this.client = new NetcatClient();
this.client.unixSocket(ipcPath)
Expand Down
60 changes: 45 additions & 15 deletions src/lib/utils/host.ts
Original file line number Diff line number Diff line change
@@ -1,30 +1,59 @@
const isDocker = (() => {
let isDockerProcess;
const {execSync} = require("child_process");
const {anchoredValue, DAPP_PATH} = require("../core/env");
const {hostname} = require("os");

const dappPath = anchoredValue(DAPP_PATH);

const hostname = require("os").hostname();
const pattern = new RegExp(
"[0-9]+\:[a-z_-]+\:\/docker\/" + hostname + "[0-9a-z]+", "i",
);
function subdir(pdir_, dir_) {
let pdir = path.resolve(path.normalize(pdir_)) + (path.sep || "/");
const dir = path.resolve(pdir, path.normalize(dir_));
if (pdir === "//") { pdir = "/"; }
if (pdir === dir) { return false; }
return dir.slice(0, pdir.length) === pdir;
}

const isDocker = (() => {
// assumption: an Embark container is always a Linux Docker container, though
// the Docker host may be Linux, macOS, or Windows
if (process.platform !== "linux") { return false; }
try {
return (
new RegExp(`[0-9]+\:[a-z_-]+\:\/docker\/${hostname()}[0-9a-z]+`, "i")
).test(
execSync(
"cat /proc/self/cgroup",
{stdio: ["ignore", "pipe", "ignore"]},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelsbradleyjr just out of curiosity: why are we doing ignore, pipe, ignore here and not just pipe?

Copy link
Contributor Author

@michaelsbradleyjr michaelsbradleyjr Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the execSync calls in this module, all we care about is stdout, so it seems more correct to 'ignore' stdin and stderr. It would, though, work with the default: 'pipe', which is equivalent to ['pipe', 'pipe', 'pipe'].

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, that makes sense!

).toString(),
);
} catch (e) {
return false;
}
})();

const isDappMountedFromWindowsDockerHost = (() => {
if (!isDocker) { return false; }
jrainville marked this conversation as resolved.
Show resolved Hide resolved
try {
isDockerProcess = require("child_process")
.execSync(
"cat /proc/self/cgroup",
return execSync(
"mount",
{stdio: ["ignore", "pipe", "ignore"]},
)
.toString().match(pattern) !== null;
.toString()
.split("\n")
.filter((line) => /nounix/.test(line))
.some((line) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some cosmetics that can be ignored: we can get rid of the parenthesis here in filter() and some

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't have them originally but tslint said it was an error to not use parens around line.

Copy link
Contributor Author

@michaelsbradleyjr michaelsbradleyjr Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When removed, I get something like this:

$ yarn lint:ts
yarn run v1.12.3
$ tslint -c tslint.json 'src/**/*.ts'

ERROR: src/lib/utils/host.ts[42, 15]: Parentheses are required around the parameters of an arrow function definition

error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Maybe it's something we can relax in the linter, and/or we could have another team discussion about adopting use of the prettier tool?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow I'm surprised tslint is complaining about that. Definitely something we should relax in the future.

const mount = line.match(/on (\/.*) type/)[1];
return mount === dappPath || subdir(mount, dappPath);
});
} catch (e) {
isDockerProcess = false;
return false;
}

return isDockerProcess;
})();

const defaultHost = isDocker ? "0.0.0.0" : "localhost";

// when we"re runing in Docker, we can expect (generally, in a development
// when we're runing in Docker, we can expect (generally, in a development
// scenario) that the user would like to connect to the service in the
// container via the **host"s** loopback address, so this helper can be used to
// container via the **host's** loopback address, so this helper can be used to
// swap 0.0.0.0 for localhost in code/messages that pertain to client-side
function canonicalHost(host: string): string {
return isDocker && host === "0.0.0.0" ? "localhost" : host;
Expand All @@ -41,5 +70,6 @@ export {
defaultCorsHost,
defaultHost,
dockerHostSwap,
isDappMountedFromWindowsDockerHost,
isDocker,
};
7 changes: 7 additions & 0 deletions src/lib/utils/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,12 @@ function sha3(arg) {
return Web3.utils.sha3(arg);
}

function sha512(arg) {
const crypto = require('crypto');
const hash = crypto.createHash('sha512');
return hash.update(arg, 'utf8').digest('hex');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, just as a precaution, you want to make sure that arg is a string, so we might want it to read

return hash.update(arg.toString(), 'utf8').digest('hex');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://nodejs.org/dist/latest-v10.x/docs/api/crypto.html#crypto_hash_update_data_inputencoding

Maybe we should just throw if it's not a string?

> ({a: 1}).toString()
'[object Object]'
> ({a: 2}).toString()
'[object Object]'

}

function soliditySha3(arg) {
const Web3 = require('web3');
return Web3.utils.soliditySha3(arg);
Expand Down Expand Up @@ -640,6 +646,7 @@ module.exports = {
getExternalContractUrl,
toChecksumAddress,
sha3,
sha512,
soliditySha3,
normalizeInput,
buildUrl,
Expand Down