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

ENV needs to be a true proxy to getenv rather than a Hash #3924

Open
headius opened this issue May 25, 2016 · 5 comments
Open

ENV needs to be a true proxy to getenv rather than a Hash #3924

headius opened this issue May 25, 2016 · 5 comments

Comments

@headius
Copy link
Member

headius commented May 25, 2016

Environment

JRuby, all versions.

Expected Behavior

In MRI, ENV is not a Hash but a pseudo-Hash-like singleton object with all of its own methods. This is done so that all Hash-y operations actually go directly to and from the system environment via getenv and setenv.

Actual Behavior

In JRuby, we have the following differences:

  • ENV is a subclass of Hash
  • ENV overrides a few Hash methods to handle case-insensitivity, ArgumentError: string contains null byte #3907 PATH encoding, and so on.
  • ENV is populated at boot by first creating a Map of RubyString, and then by inserting that Map's contents into ENV itself.

This allows us to delegate some of ENV's logic to Hash, but also results in a lot of needless complexity and a visible difference from MRI.

In the case of ENV_JAVA, we do basically the same process, when we really could just go directly to/from System.getProperty/setProperty.

I propose that we fix this in a 9k release by making ENV truly just be a proxy as in MRI.

There is one possible side effect of following MRI's course: We would not cache the Ruby String instances that ENV produces, and would have to transcode to/from Java String to interact with env and property APIs.

@kares
Copy link
Member

kares commented Jun 12, 2018

JRuby has been using SystemPropertiesMap for a while now - somewhere along 9.0 or so

@kares kares closed this as completed Jun 12, 2018
@kares
Copy link
Member

kares commented Jun 12, 2018

oh sorry - this is about ENV not ENV_JAVA

@kares kares reopened this Jun 12, 2018
@headius
Copy link
Member Author

headius commented Jun 12, 2018

It still scares me a bit to actually twiddle the process ENV but it would make one big feature a lot simpler: environment changes during spawning.

Currently the only way we can implement those changes is to tweak the command line to use sh -c and set environment variables along the way. This obviously is unexpected when running a simple command and there are multiple bugs in the tracker related to this behavior difference.

We also have a growing separation between "JRuby at the command line" and "JRuby embedded in a larger app"; should the behavior change depending on which mode you're running in, with the CLI mode being most POSIXy and compliant?

@kares
Copy link
Member

kares commented Jun 13, 2018

We also have a growing separation between "JRuby at the command line" and "JRuby embedded in a larger app"; should the behavior change depending on which mode you're running in, with the CLI mode being most POSIXy and compliant?

oh right, that is a concern - sounds to me like it definitely should change (e.g. would not expect setting native env in Tomcat). maybe leaving it as is for now - as there seems to be no user complaints atm.

@headius
Copy link
Member Author

headius commented Jun 13, 2018

there seems to be no user complaints

Well, other than the issues people have spawning subprocesses with env changes. There's at least a few of those bugs. Our "hacky" workaround of turning the command into a shell script works reasonably well.

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

No branches or pull requests

2 participants