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

michaelsbradleyjr · 2018-12-18T04:02:25Z

When embark is running on Linux and macOS, unix socket files are used for geth's and embark's IPC files. If the context is a Linux container running on a Windows Docker host, and if the DApp is mounted from the host's file system, there is a problem: unix socket files are incompatible with the Windows file system.

Detect the problematic scenario and use a path outside of the mounted directory for embark's and geth's IPC files.

To avoid a circular dependency problem, remove the utils/utils.js dependency from core/env.js, don't have utils/host.ts depdend on core/fs.js, and implement the ipcPath() helper as a static method on the class exported from core/ipc.js.

0x-r4bbit

Codewise this looks pretty solid!

Left some minor comments.

0x-r4bbit · 2018-12-18T10:18:01Z

src/lib/core/env.js

@@ -2,7 +2,7 @@

 const {execSync} = require('child_process');
 const {delimiter} = require('path');
-const {joinPath} = require('../utils/utils.js');
+const {join} = require('path');


👍 great move!

Also, can we combine the required symbols for path? There's another one already for delimiter.

Good catch, will combine them.

0x-r4bbit · 2018-12-18T10:23:48Z

src/lib/utils/host.ts

+    ).test(
+      execSync(
+        "cat /proc/self/cgroup",
+        {stdio: ["ignore", "pipe", "ignore"]},


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

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'].

Yea, that makes sense!

0x-r4bbit · 2018-12-18T10:27:21Z

src/lib/utils/host.ts

+      .toString()
+      .split("\n")
+      .filter((line) => /nounix/.test(line))
+      .some((line) => {


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

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

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?

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

src/lib/utils/host.ts

When embark is running on Linux and macOS, unix socket files are used for geth's and embark's IPC files. If the context is a Linux container running on a Windows Docker host, and if the DApp is mounted from the host's file system, there is a problem: unix socket files are incompatible with the Windows file system. Detect the problematic scenario and use a path outside of the mounted directory for embark's and geth's IPC files. To avoid a circular dependency problem, remove the `utils/utils.js` dependency from `core/env.js`, don't have `utils/host.ts` depdend on `core/fs.js`, and implement the `ipcPath()` helper as a static method on the class exported from `core/ipc.js`.

andremedeiros · 2018-12-19T22:53:56Z

src/lib/core/env.js

@@ -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');


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.

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.

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

andremedeiros · 2018-12-19T22:58:27Z

src/lib/core/ipc.js

+      return `\\\\.\\pipe\\${basename}`;
+    } else if (isDappMountedFromWindowsDockerHost) {
+      ipcDir = utils.joinPath(
+        os.tmpdir(), `embark-${utils.sha512(fs.dappPath()).slice(0, 8)}`


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

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.

andremedeiros · 2018-12-19T23:00:53Z

src/lib/utils/utils.js

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


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');

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]'

michaelsbradleyjr · 2018-12-20T01:00:49Z

Closing in favor of a forthcoming PR that makes saving the .ipc files within os.tmpdir() the default behavior.

This PR replaces #1202. When embark is running on Linux and macOS, unix socket files are used for geth's and embark's IPC files. A problem can arise if a DApp is in a deeply nested directory structure such that the character-length of the path to a socket file exceeds the maximum supported length. See #450. Also, if the DApp context is a Linux container running on a Windows Docker host, and if the DApp is mounted from the host's file system, there is a problem: unix socket files are incompatible with the Windows file system. Solve both problems at once by storing a DApp's `.ipc` files in a directory within `os.tmpdir()`. Use a truncated sha512 hash of the DApp's path in the name of the temporary directory created for that purpose so that DApps won't collide (with an acceptably low probability of collision). Remove the `utils/utils.js` dependency from `core/env.js` and implement the `ipcPath()` helper as a static method on the class exported from `core/ipc.js`.

michaelsbradleyjr · 2018-12-20T02:03:05Z

Replaced by #1214.

This PR replaces #1202. When embark is running on Linux and macOS, unix socket files are used for geth's and embark's IPC files. A problem can arise if a DApp is in a deeply nested directory structure such that character-length of the path to a socket file exceeds the maximum supported length. See #450. Also, if the DApp context is a Linux container running on a Windows Docker host, and if the DApp is mounted from the host's file system, there is a problem: unix socket files are incompatible with the Windows file system. Solve both problems at once by storing a DApp's `.ipc` files in a directory within `os.tmpdir()`. Introduce `ipcPath()` in `core/fs.js` and use a truncated SHA-512 hash of the DApp's path in the name of the temporary directory created for that purpose so that DApps won't collide (with an acceptably low probability of collision).

michaelsbradleyjr requested a review from a team December 18, 2018 04:02

0x-r4bbit suggested changes Dec 18, 2018

View reviewed changes

jrainville reviewed Dec 18, 2018

View reviewed changes

src/lib/utils/host.ts Show resolved Hide resolved

michaelsbradleyjr force-pushed the refactor/ipc-location branch 4 times, most recently from f6bbbb1 to 6f219e2 Compare December 18, 2018 18:43

michaelsbradleyjr force-pushed the refactor/ipc-location branch from 6f219e2 to af79595 Compare December 18, 2018 20:39

0x-r4bbit approved these changes Dec 19, 2018

View reviewed changes

andremedeiros reviewed Dec 19, 2018

View reviewed changes

andremedeiros approved these changes Dec 19, 2018

View reviewed changes

michaelsbradleyjr closed this Dec 20, 2018

michaelsbradleyjr mentioned this pull request Dec 20, 2018

feat(@embark/core): store IPC files in a dir within os.tmpdir() #1214

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

michaelsbradleyjr commented Dec 18, 2018

0x-r4bbit left a comment

0x-r4bbit Dec 18, 2018

michaelsbradleyjr Dec 18, 2018

michaelsbradleyjr Dec 18, 2018

0x-r4bbit Dec 18, 2018

michaelsbradleyjr Dec 18, 2018 •

edited

Loading

0x-r4bbit Dec 19, 2018

0x-r4bbit Dec 18, 2018

michaelsbradleyjr Dec 18, 2018

michaelsbradleyjr Dec 18, 2018 •

edited

Loading

0x-r4bbit Dec 19, 2018

andremedeiros Dec 19, 2018

michaelsbradleyjr Dec 19, 2018

jrainville Dec 20, 2018

andremedeiros Dec 19, 2018

michaelsbradleyjr Dec 19, 2018

andremedeiros Dec 19, 2018

michaelsbradleyjr Dec 19, 2018

michaelsbradleyjr commented Dec 20, 2018

michaelsbradleyjr commented Dec 20, 2018

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

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

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

Conversation

michaelsbradleyjr commented Dec 18, 2018

0x-r4bbit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelsbradleyjr Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelsbradleyjr Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelsbradleyjr commented Dec 20, 2018

michaelsbradleyjr commented Dec 20, 2018

michaelsbradleyjr Dec 18, 2018 •

edited

Loading

michaelsbradleyjr Dec 18, 2018 •

edited

Loading