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.

Sign up for GitHub

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

Partially call site.process on production #294

Closed
wants to merge 4 commits into from
Closed

Partially call site.process on production #294

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10b2525
fix race
mertkahyaoglu Feb 11, 2017
6ad772c
Merge branch 'master' into fix-race
mertkahyaoglu Feb 12, 2017
c455371
Merge remote-tracking branch 'origin/master' into fix-race
mertkahyaoglu Feb 22, 2017
ebf76e5
Merge branch 'master' into fix-race
mertkahyaoglu Feb 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/jekyll-admin/server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,11 @@ def write_file(path, content)
File.open(path, "wb") do |file|
file.write(content)
end
site.process
if ENV["RACK_ENV"]
site.process
else
site.read
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what are you trying to do here? I'm afraid I'm having trouble following.

Copy link
Member Author

@mertkahyaoglu mertkahyaoglu Feb 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying try to eliminate the overlapping parts between two threads (regeneration and api server).

Does incremental regeneration call any methods from site.process ? If so, calling them again after writing to a file might cause a confusion. Do we really need to call all methods of site.process, rather than just a few ones that are really necessary for the API ? Because when I use only site.read, everything seems to work.

The if statement there is for tests to pass.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parkr See #289 (comment) for context. Is the race condition in the write? In the read? In the generate? Any way to unthread things? We need to write and generate so previews work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really a race condition in the traditional sense. We boot up a Watcher from jekyll-watch which runs site.process every time a file saved. The issue here appears to be: if opts["watch"] && opts["serving"] are passed to Jekyll, then the watcher in another thread runs site.process and you're also trying to run site.process at the same time and it can cause confusion. I think if there's another Jekyll process running, the call to site.process is not necessary here at all. Can you get away with just calling site.read?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you get away with just calling site.read?

Will that introduce a race condition if the file isn't yet written to disk?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will that introduce a race condition if the file isn't yet written to disk?

Yeah, I encountered that once but comparing with the current version, this is an improvement 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if, instead, we disable watch and manually call site.process (which should then be blocking). As of right now, each write will require two reads (and I believe now, two full processes).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. However I have no idea how to disable it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parkr any bright ideas? We'd need to force watch to false. Perhaps the same way we do for the Pages gem?

end

def delete_file(path)
Expand Down