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

Gracefully handle HOME access failure? #19

Closed
brycefisher opened this issue May 27, 2015 · 16 comments
Closed

Gracefully handle HOME access failure? #19

brycefisher opened this issue May 27, 2015 · 16 comments

Comments

@brycefisher
Copy link

Related to shipitjs/shipit#70 and #13 , if HOME is not accessible, this can cause upstream modules that require liftoff to fail (even if they don't explicitly need any specific v8flags).

Is it possible to issue a warning and keep going if v8flags fails for some reason? The specific case I ran into today is where the uid executing the node process doesn't have access to the $HOME directory.

@tkellen
Copy link

tkellen commented May 27, 2015

Thanks @brycefisher! This module has gone through a tremendous amount of churn to cover all the possible edge cases of writing a silly configuration file. Yours is the first failure in some time. Rather than have v8flags return an empty array of options in a failure case (or have Liftoff ignore v8flags throwing), I'd like to fix the underlying cause.

Could you confirm for me if the uid has write access to the temp directory that results from running this? If it does, I think I know what needs to be done.

function tmpdir() {
  if (process.platform === 'win32') {
    return process.env.TEMP ||
           process.env.TMP ||
           (process.env.SystemRoot || process.env.windir) + '\\temp';
  } else {
    return process.env.TMPDIR ||
           process.env.TMP ||
           process.env.TEMP ||
           '/tmp';
  }
};

@tkellen
Copy link

tkellen commented May 27, 2015

PS: this is from the os module, so you can also do require('os').tmpdir()

@brycefisher
Copy link
Author

Yes! We've resolved that issue by modifying the environment, but I'll try to recreate via a docker image tonight. Thanks!

@tkellen
Copy link

tkellen commented May 28, 2015

Sounds good! The current scenario where a temporary directory is used rather than a home directory is when no home directory can be found. I believe we can resolve this by instead checking for write access to the home dir. This will fail in the current case (if no home directory is found, of course you can't write to it) and your case, (having a home directory but not being able to write to it). Obviously the failover only works if you can actually write to the tmpdir, though.

Honestly, it seems a bit nonsensical that you actually have a value for the home directory but you can't write to it? How is that happening?

@brycefisher
Copy link
Author

Yeah it was a little weird. We had an ec2 instance running jenkins. Jenkins had a separate user, but apparently no one had configured the home directory properly. I was a little sad that having the home directory setup properly would cause a shipit dependency to fail, but it is a bit of edge case.

@brycefisher
Copy link
Author

Despite my best efforts at this, I'm unable to reproduce the issue we ran into in my docker image:
https://github.com/brycefisher/js-v8flags-no-home-dir

Running require('os').tmpdir() inside that image gave /tmp; unsurprisingly the unpriv user can access /tmp. Sooooo I'm thinking maybe that user didn't have access to /tmp as well... 💥

I'll investigate further from the office tomorrow 🔬

@tkellen
Copy link

tkellen commented May 28, 2015

Thanks for investigating @brycefisher. Standing by for more.

@brycefisher
Copy link
Author

Okay, I did a little more digging and it looks like the tmpdir is fine:

screen shot 2015-05-28 at 10 22 55 am

screen shot 2015-05-28 at 10 14 32 am

However, the failures happened when the $HOME directory pointed at a non-existent directory. I'm trying to test that situation now to see if I can reproduce the issues we had.

@brycefisher
Copy link
Author

Hmm tried to reproduce the situation locally, and I still wasn't able to exactly reproduce this issue:

index.js

var v8flags = require('v8flags');

v8flags(function (err, results) {
  console.log(results);  // [ '--use_strict',
                         //   '--es5_readonly',
                         //   '--es52_globals',
                         //   '--harmony_typeof',
                         //   '--harmony_scoping',
                         //   '--harmony_modules',
                         //   '--harmony_proxies',
                         //   '--harmony_collections',
                         //   '--harmony',
                         // ...
});

bash:

$ ls -lah /usr/lib/no-exist
ls: /usr/lib/no-exist: No such file or directory
$ HOME=/usr/lib/no-exist node index.js
undefined

@tkellen
Copy link

tkellen commented May 28, 2015

However, the failures happened when the $HOME directory pointed at a non-existent directory. I'm trying to test that situation now to see if I can reproduce the issues we had.

I suspected that was the problem. We can definitely fix it. It's not enough to detect if a home path can be found in the environment, we also have to ensure you can write to it (something v8flags currently takes for granted).

@brycefisher
Copy link
Author

Yay! I was getting hopeless about reproducing this.

@brycefisher
Copy link
Author

Is there a big impact on performance to check for write access?

@tkellen
Copy link

tkellen commented May 28, 2015

Nah, it should be fine.

@elldritch
Copy link
Contributor

@brycefisher, I just pushed 2.0.11 which should resolve this issue. Can you see if this fix works for you?

@brycefisher
Copy link
Author

Woot! Sadly, I've left that project and blew away my whole setup. Thanks so much for fixing this!!

@elldritch
Copy link
Contributor

Sure thing. Looks like this seems to be working for people from #22, so I'm going to go ahead and close this issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants