Disable watch to prevent race condition between threads calling site.process #320

parkr · 2017-03-09T19:42:49Z

This prevents a race condition with another thread calling site.process.
Before, site.process had to complete before the response would be
correct, which is quite unreliable. Instead, use the appropriate Jekyll
classes to read the item.

Replaces #294.

This prevents a race condition with another thread calling site.process. Before, site.process had to complete before the response would be correct, which is quite unreliable. Instead, use the appropriate Jekyll classes to read the item.

mertkahyaoglu

Wow 😄 We were really overthinking. Great solution 👍

benbalter

I think we might need to do this for static files and data files as well.

benbalter · 2017-03-09T19:47:51Z

lib/jekyll-admin/server/page.rb

@@ -20,7 +20,9 @@ class Server < Sinatra::Base
        end

        write_file(write_path, page_body)
-        updated_page = pages.find { |p| p.path == write_path }
+        updated_page = Jekyll::Page.new(
+          site, site.source, File.dirname(write_path), File.basename(write_path)


Is the path sanitized prior to being read? (to prevent directory traversal from the user-supplied write_path?

@benbalter The write_path comes from either document_path or request_path. In my opinion this should most certainly be sanitized before it gets here as it's used to interact with the filesystem before this line.

mertkahyaoglu · 2017-03-09T20:07:12Z

lib/jekyll-admin/server/collection.rb

-        updated_document = collection.docs.find { |d| d.path == write_path }
+        updated_document = Jekyll::Document.new(write_path, {
+          :collection => collection, :site => site,
+        }).tap(&:read)
        render_404 if updated_document.nil?


If we rename updated_document to document, we can use ensure_document which does the same thing. Same for ensure_page.

We won't be able to do this, because ensure_document calls document, which uses the buggy way of reading:

80 def document 81 collection.docs.find { |d| d.path == document_path } 82 end

Isn't document overwritten by Jekyll::Document.new ? So ensure_document uses newly created document object instead of the result object of document method.

I'm guessing methods' priority is higher over objects in ruby so it will use document method anyway?

parkr · 2017-03-09T20:29:44Z

I think we might need to do this for static files and data files as well.

Static files, yes, but data files use their own API: DataFile.new(params["data_file_id"]) which reads in the file manually.

…back to the old way.

mertkahyaoglu · 2017-03-09T20:34:33Z

lib/jekyll-admin/server/collection.rb

+        collection.docs.find { |d| d.path == document_path } ||
+          Jekyll::Document.new(write_path, {
+            :collection => collection, :site => site,
+          }).tap(&:read)


This is really 🆒 You da boss 😎

mertkahyaoglu · 2017-03-09T20:38:59Z

lib/jekyll-admin/server/page.rb

@@ -85,7 +84,9 @@ def relative_page_path
      end

      def page
-        site.pages.find { |p| sanitized_path(p.path) == page_path }
+        Jekyll::Page.new(


Missed || here or is this on purpose?

You could do either.

I think he's asking if we want to look to the existing page first then fall back to creating a new one like we do for other content types?

mertkahyaoglu · 2017-03-10T15:55:15Z

lib/jekyll-admin/server/collection.rb

@@ -29,8 +29,7 @@ class Server < Sinatra::Base
        end

        write_file(write_path, document_body)
-        updated_document = collection.docs.find { |d| d.path == write_path }
-        render_404 if updated_document.nil?
+        ensure_document
        json updated_document.to_api(:include_content => true)


This needs to be document now instead of updated_document.

mertkahyaoglu · 2017-03-10T15:55:40Z

lib/jekyll-admin/server/page.rb

@@ -20,8 +20,7 @@ class Server < Sinatra::Base
        end

        write_file(write_path, page_body)
-        updated_page = pages.find { |p| p.path == write_path }
-        render_404 if updated_page.nil?
+        ensure_page
        json updated_page.to_api(:include_content => true)


This needs to be page.

mertkahyaoglu · 2017-03-11T22:47:50Z

This should be good to go after deleting test files (unless failing tests generating them).

parkr · 2017-03-12T01:21:56Z

This should be good to go after deleting test files (unless failing tests generating them).

@mertkahyaoglu Would you mind taking this PR and running with it? I am not able to commit any more time to it unfortunately but I think we have the right approach here. 😄

mertkahyaoglu · 2017-03-12T08:56:40Z

@parkr Got it 👍

I reverted 1203e14 because write_path becomes undefined in page, document methods.

Another thing is that static files tests are failing when JekyllAdmin::StaticFile.new is used.

Here are the failing tests;

rspec ./spec/jekyll-admin/server/static_file_spec.rb:30 # static_files 404s when a static file doesn't exist
rspec ./spec/jekyll-admin/server/static_file_spec.rb:35 # static_files returns a deep directory listing
rspec ./spec/jekyll-admin/server/static_file_spec.rb:51 # static_files returns a directory listing starting only at the root

@benbalter Any solution for this? Or shall we revert this approach for static files.

benbalter · 2017-03-13T13:28:56Z

@mertkahyaoglu: @parkr is out of the office this week, but I'll take a look this morning and see what I can do here to get the build passing.

mertkahyaoglu · 2017-03-13T17:16:51Z

lib/jekyll-admin/server/page.rb

+        return found if found
+        return unless File.file?(write_path)
+        Jekyll::Page.new(
+          site, site.source, File.dirname(write_path), File.basename(write_path)


Do we want the full path for dir ?

ashmaroli · 2017-03-15T19:52:45Z

For the test, can we write to a file manually then expect the regeneration to not happen? I don't know how we can check whether the regeneration occurs doing that.

sent a PR #324 inspired by this comment.

test --watch with this plugin

benbalter · 2017-03-15T21:06:21Z

Full props go to @ashmaroli. If tests are green, I believe this can be merged.

mertkahyaoglu · 2017-03-15T21:13:33Z

@ashmaroli @benbalter @parkr Awesome work guys. Many thanks. This and #323 deserve a release.

ashmaroli · 2017-03-16T02:19:05Z

Thank you for the acknowledgement @benbalter and @mertkahyaoglu 😃 🍻

ashmaroli · 2017-03-16T02:37:48Z

This and #323 deserve a release.

I feel #321 should be addressed by the next release. Either with the merge of #322 (or another better implementation), or an entry to the documentation informing users about the situation.

Another TODO:

I believe the only thing remaining here would be to document the limitation.

from Ben's comment above.

Document #320

mariusa · 2017-07-22T13:53:12Z

This introduces other issues, see #340

Not sure why the admin has to take over re-generation when enabled as a plugin? Just write files to disk, like all editors. Then jekyll would do it's job and re-generate content on changes, no matter if files are changed via admin or other editors.

Thanks

ashmaroli · 2017-07-22T14:29:34Z

From Jekyll Admin's online docs

Note: Since there are two Sinatra servers that might call site.process concurrently, Jekyll Admin disables --watch flag to prevent a race condition between these servers that might cause incorrect responses for the API.
This ensures that the site is regenerated by only the process that Jekyll Admin runs.

mariusa · 2017-07-22T16:10:50Z

The current editing of .css, layouts or includes is a pain. We have to stop jekyll and start again on each css edit, to see changes.
What solutions do you recommend?

The simplest one does see to limit jekyll-admin to edit files as any IDE, just write them to local disk. Don't try to generate anything, jekyll already handles that well, and it works for all assets.

ashmaroli · 2017-07-22T16:19:32Z

The simplest solution when editing "non-content files" is to comment out jekyll-admin from your _config.yml or Gemfile depending on how its being used..

mariusa · 2017-07-22T16:41:25Z

So

edit some pages in jekyll-admin
realize we need to edit some CSS/JS/include
stop jekyll
comment out jekyll-admin
start jekyll
multiple CSS edits
stop jekyll
uncomment jekyll-admin
start jekyll
back to page editing

Would you agree it's cumbersome?

ashmaroli · 2017-07-22T16:46:34Z

Would you agree it's cumbersome?

Definitely.
I'm planning to release a gem-version of my fork of jekyll-admin soon.. It'll support editing css and other non-content files via the interface.. (still disables --watch though..)

mertkahyaoglu · 2017-07-23T11:38:32Z

Actually I'm planning to bring the watch back. With the changes I described here, the possibility of hitting a race condition is very low (%0 for me, please test #415 locally and let me know if you encounter a race condition. It will work in dev mode for you to test). If the possibility is acceptable, I think we should give it a shot.

ashmaroli · 2017-07-24T10:38:01Z

I'm planning to release a gem-version of my fork

FYI, released under gem-name jekyll-manager

parkr requested review from mertkahyaoglu and benbalter March 9, 2017 19:42

mertkahyaoglu approved these changes Mar 9, 2017

View reviewed changes

benbalter reviewed Mar 9, 2017

View reviewed changes

mertkahyaoglu requested changes Mar 9, 2017

View reviewed changes

Use ensure_* helper methods and use this more robust method as a fall…

1203e14

…back to the old way.

mertkahyaoglu approved these changes Mar 9, 2017

View reviewed changes

mertkahyaoglu reviewed Mar 9, 2017

View reviewed changes

mertkahyaoglu reviewed Mar 10, 2017

View reviewed changes

mertkahyaoglu mentioned this pull request Mar 10, 2017

Partially call site.process on production #294

Closed

parkr and others added 2 commits March 11, 2017 16:55

Use accurate method names; add the || for the page server back.

9e7b78d

Merge branch 'master' into prevent-race-condition-on-change

efd6ede

benbalter added 3 commits March 13, 2017 12:20

centralize path manipulation methods in server.rb

4ce5082

slightly less abstraction

c43c34e

DRY up removing the site source

0c170bd

mertkahyaoglu reviewed Mar 13, 2017

View reviewed changes

benbalter added 7 commits March 13, 2017 13:55

move path helper methods to its own module

82aba50

correct rubocop offenses;

a573bb5

centralize file manipulation methods in file helper

87f8e2b

autoload ALL THE THINGS!

9514901

add some missing tests

2bf85ae

use FileHelper and PathHelper for DataFile methods

3652c41

add entries url to collection hash

f4d94ab

test --watch with this plugin

1f8c8c2

ashmaroli mentioned this pull request Mar 15, 2017

test --watch with this plugin #324

Merged

Merge pull request #324 from ashmaroli/test-watch-admin

bce59f7

test --watch with this plugin

mertkahyaoglu merged commit 6dd908b into master Mar 15, 2017

mertkahyaoglu deleted the prevent-race-condition-on-change branch March 15, 2017 21:19

This was referenced Mar 15, 2017

Post editor exiting fullscreen on save #305

Closed

Race condition after creating files #289

Closed

Liquid Renderer NIL CASE crash #220

Closed

mertkahyaoglu changed the title ~~When updating a document or page, read item by itself~~ Disable watch to prevent race condition between threads calling site.process Mar 16, 2017

benbalter mentioned this pull request Mar 20, 2017

API for Dashboard #329

Closed

mertkahyaoglu added a commit that referenced this pull request Mar 22, 2017

document #320

b7c14c9

mertkahyaoglu added a commit that referenced this pull request Mar 22, 2017

Merge pull request #331 from jekyll/docs

e3e77e0

Document #320

mertkahyaoglu mentioned this pull request Mar 26, 2017

Release 0.4.1 #334

Merged

jekyllbot added the has-pull-request label Mar 26, 2017

mertkahyaoglu mentioned this pull request Mar 31, 2017

add jekyll-admin by default jekyll/jekyll#5783

Closed

enkota mentioned this pull request Apr 8, 2017

Regeneration of _includes files #340

Open

pedrolamas mentioned this pull request Apr 12, 2017

Running jekyll on Bash on Ubuntu on Windows with --watch does nothing! jekyll/jekyll#6023

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable watch to prevent race condition between threads calling site.process #320

Disable watch to prevent race condition between threads calling site.process #320

parkr commented Mar 9, 2017

mertkahyaoglu left a comment

benbalter left a comment

benbalter Mar 9, 2017

parkr Mar 9, 2017 •

edited

Loading

mertkahyaoglu Mar 9, 2017

parkr Mar 9, 2017

mertkahyaoglu Mar 9, 2017

mertkahyaoglu Mar 9, 2017

parkr commented Mar 9, 2017

mertkahyaoglu Mar 9, 2017

mertkahyaoglu Mar 9, 2017 •

edited

Loading

parkr Mar 9, 2017

benbalter Mar 9, 2017

mertkahyaoglu Mar 10, 2017

mertkahyaoglu Mar 10, 2017 •

edited

Loading

mertkahyaoglu commented Mar 11, 2017 •

edited

Loading

parkr commented Mar 12, 2017

mertkahyaoglu commented Mar 12, 2017

benbalter commented Mar 13, 2017

mertkahyaoglu Mar 13, 2017

ashmaroli commented Mar 15, 2017

benbalter commented Mar 15, 2017

mertkahyaoglu commented Mar 15, 2017 •

edited

Loading

ashmaroli commented Mar 16, 2017

ashmaroli commented Mar 16, 2017

mariusa commented Jul 22, 2017

ashmaroli commented Jul 22, 2017

mariusa commented Jul 22, 2017

ashmaroli commented Jul 22, 2017

mariusa commented Jul 22, 2017

ashmaroli commented Jul 22, 2017

mertkahyaoglu commented Jul 23, 2017

ashmaroli commented Jul 24, 2017

Disable watch to prevent race condition between threads calling site.process #320

Disable watch to prevent race condition between threads calling site.process #320

Conversation

parkr commented Mar 9, 2017

mertkahyaoglu left a comment

Choose a reason for hiding this comment

benbalter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr commented Mar 9, 2017

Choose a reason for hiding this comment

mertkahyaoglu Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mertkahyaoglu Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

mertkahyaoglu commented Mar 11, 2017 • edited Loading

parkr commented Mar 12, 2017

mertkahyaoglu commented Mar 12, 2017

benbalter commented Mar 13, 2017

Choose a reason for hiding this comment

ashmaroli commented Mar 15, 2017

benbalter commented Mar 15, 2017

mertkahyaoglu commented Mar 15, 2017 • edited Loading

ashmaroli commented Mar 16, 2017

ashmaroli commented Mar 16, 2017

mariusa commented Jul 22, 2017

ashmaroli commented Jul 22, 2017

mariusa commented Jul 22, 2017

ashmaroli commented Jul 22, 2017

mariusa commented Jul 22, 2017

ashmaroli commented Jul 22, 2017

mertkahyaoglu commented Jul 23, 2017

ashmaroli commented Jul 24, 2017

parkr Mar 9, 2017 •

edited

Loading

mertkahyaoglu Mar 9, 2017 •

edited

Loading

mertkahyaoglu Mar 10, 2017 •

edited

Loading

mertkahyaoglu commented Mar 11, 2017 •

edited

Loading

mertkahyaoglu commented Mar 15, 2017 •

edited

Loading