Remove Dir.chdir calls from Stasis and Stasis::Controller #52

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

This wreaks havoc if you are embedding Stasis for HTML generation within a larger generation process, e.g., generating static blog entries like:

Article.all.each do |article|
  stasis = Stasis.new("templates/", "public/articles/#{article.id}")
  stasis.render("article.html.haml", params: { article: article })
end

This will fail on the second iteration, since the current directory will now be "templates/"!

Having removed the calls to chdir, all the specs continue to pass, as well as my own generator specs. Is there a reason why this might break something else?

If there is a reason Dir.chdir is needed, we should probably be passing it a block so that the original directory is restored afterwards.

You should be able to change this to

view = nil
Dir.chdir(File.dirname(@path)) do

and end the block at line 218.

@winton

What case is the chdir covering? Is it only for a relative-path require call? Or something else?

The chdir block would work, though the documentation warns:

chdir blocks can be nested, but in a multi-threaded program an error will
be raised if a thread attempts to open a chdir block while another thread has one open.

That means that stasis projects couldn't be generated inside of, say, Sidekiq (which uses thread pools for queue/worker processing), or any other threaded environment.

I'm not sure if it has any use for Stasis. Since the tests passed with these disabled, I would guess not.

It is there mostly as a convenience to the end user. When they are writing code in a controller, they can use relative paths the same way they would if controller.rb were being executed instead of eval'ed.

Unfortunately stasis already has some other code that is not thread-safe. I'm not saying that shouldn't be a goal, but it should maybe be apart of a broader project.

This block would only need to wrap around the instance_eval below it.

I wouldn't mind digging into both the #chdir calls and the thread-safety, if you think that's something worth pursuing.

I had used Stasis before for some static sites, and thought of it again when I needed to do some dynamic site generation (pass some objects and render the templates into a given output directory). The stasis interface works quite well for this, but I don't imagine this is a common use case. That said, it still seems like a perfect fit -- stasis has a good amount of power, but is simple enough to be embedded in a way that, say, Middleman/Jekyll/... aren't.

If you're interested, I'll throw away this pull request and start up a feature branch for safely embedding Stasis in a larger system.

raine commented Apr 19, 2013

p Dir.pwd # my-app/
stasis = Stasis.new 'site/'
stasis.render
p Dir.pwd # my-app/site/tmp

Running in a rake task. I don't think it should have such side effects.

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