Environment cleanup #216

Closed
wants to merge 2 commits into
from

6 participants

@ghost

Leave it up to OTP instead of trying to mimic the behavior (and only partially at that)

Rob added some commits Jan 20, 2014
Rob testcase for broken environment cleanup 4935d81
Rob cleanup env by unloading App instead of recreating
It is safer and easier to let OTP handle resetting of the environment
back to its original state, rather than trying to mimic this behavior.
On top of that, the attempted behavior did not take into account that a
configuration file can reference other configuration files.
79b372e
@tuncer

Regardless of whether we merge this patch, maybe we should use a slave node for isolation of test runs or test cases. Thoughts?

@Motiejus

This PR will enable us to test a project which depends on sys.config which includes other targets. Since curent codebase is trying to replace what application does and pull request makes it rely on OTP, I would merge this PR straight away.

@tuncer do you mean slave node for isolating test cases? I can't see real value here. In my opinion, trying to clean state by default is wrong. Teardown functions in test suites should take care of eliminating side-effects. It can be an option to enable it (then testing in slave is a good idea). However, most projects should use it only for troubleshooting 'why do my test cases fail, maybe they don't clean the state properly'.

@Motiejus Motiejus referenced this pull request in spilgames/rebar Jan 21, 2014
Merged

Improved env cleanup #2

@tuncer

@Motiejus I haven't tested the patch, but I agree that this looks like a nice way to simplify and potentially solidify the code.

do you mean slave node for isolating test cases?

Yes, and it would be similar in effect to the way we spawn ct_run in rebar_ct. Filed #217 for more discussion.

@Motiejus

This pull request is backwards compatible; it just fixes the current approach of the app environment cleanup.

Using a slave node makes sense for projects like Riak. I agree it should be done that way, but is out of scope of this pull request.

@tuncer

@Motiejus I agree.

@Vagabond

We used to use application:unload at Basho, but it would randomly crash the node, has this been fixed in OTP?

@Motiejus

@Vagabond I've never heard of that (at least on anything R14B03+, since when I started developing Erlang seriously).

We use rebar with this patch on Erlang R15B01 for quite a while now and it works perfectly. We do a few test runs per day with complex environment cleanups in our CI servers, never seen the node crash.

Ping?

@slfritchie

Yes, application:unload() would crash the entire VM now and again. So I wrote the nasty hack. Because it's far cheaper time-wise to run the nasty hack than it is to spawn a slave node and run the code from the safety of a separate VM.

If commit 209ca73 guarantees that application:unload() can permit crashes exactly 0% of the time, then fine, wonderful, go for it. But the riak_core and riak_kv EUnit test suites are brutal, and introducing false positives because cleanup code isn't exactly 100% correct and exactly 100% safe is something that I have exactly 0% interest in.

@Motiejus

@slfritchie why not remove the workaround from the build system, and fix the crash where the problem actually resides, i.e. BEAM? If you could also provide me with a test case which reproduces the crash, I will be happy to help and fix this.

Like I said, we've been using rebar with this patch for quite a while and never experienced any random VM crashes.

@tsloughter
Rebar member

@slfritchie @Vagabond any update on this?

@tsloughter tsloughter closed this Jun 15, 2014
@tuncer

@tsloughter why did you close this?

@ferd

Hi, this issue was closed in an attempt to do quick basic filtering, with the benediction of rebar project owners. These issues and pull requests are not issues or code we're spitting on, but given the burden of the task and how much code rot may have happened since these were open is unknown from maintainers at this time. All tickets prior to March 2014 were closed and will be reopened on a per-request basis if we see interest from the reporter or contributor, or if some of the issues reported are still valid after the various patches that have made it since they were opened.

This is a fairly brutal first step to help us get a proper understanding of what is still valid or not, but that has been proven efficient in the past. Sorry for the inconvenience, things should go smoother from there on.

@tuncer

I see, so was this rejected?

@ferd

No. These are not issues or code we're saying no to, it's an attempt at filtering/culling the backlog (which had over 90 issues/pull reqs combined) with many of them being untracked. We can reopen them if there is interest from the author or it's still seen as a bug/worthwhile enhancement by the community.

@Motiejus

I think the approach is valid and the pull request should be reconsidered: either to merge, or give a real case when it causes problems, so we can fix OTP.

@ferd ferd added the bug label Jun 15, 2014
@ferd

Given there is a currently failing test case for this I'm guessing you had issues with it? I've noted this one as a bug, but it looks like github has decided to make it impossible to reopen this one (Dangit github!).

I'll try to figure out how to reopen it.

Note that starting with 17.0, the function application:set_env/4 has started being able to write to configuration that will last past unloading and the approach taken here won't work in this case anymore.

@ferd

Given the pull request is no longer a good solution in 17.0 and above, I've decided we may want to track this in #298 instead.

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