Partially call site.process on production #294
Conversation
|
@parkr @benbalter Is this good to go? I would love to resolve this before the upcoming release |
| site.process | ||
| else | ||
| site.read | ||
| end |
There was a problem hiding this comment.
Can you explain what are you trying to do here? I'm afraid I'm having trouble following.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Can you get away with just calling site.read?
Will that introduce a race condition if the file isn't yet written to disk?
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
That makes sense. However I have no idea how to disable it.
There was a problem hiding this comment.
@parkr any bright ideas? We'd need to force watch to false. Perhaps the same way we do for the Pages gem?
|
Should I merge this as a temporary(not complete) solution for now ? |
|
I think we're whacking this with a way bigger stick than we need to. I chatted with @benbalter and it sounds like we can simply read in the document using the various Jekyll classes and throw it away when the request is done. I'm writing up a PR now. |
Fixes #289
As @parkr explains the problem here, the two threads (one for API and one for Jekyll) that Jekyll Admin boots up are overlapping somehow and causing a confusion in some cases, for example, writing to a file.
The solution I can think of for this problem is to call
site.processpartially since some of the methods are already called after regeneration. (correct me if I'm wrong)I only used
site.readwhich gets the site data from the filesystem and stores it in internal data structures to be able to send the newly created pages/documents to the front end properly.For the tests to pass, I added a check whether
ENVvariable is set or not. If so, it calls completesite.processsince site regeneration is not available for tests.@parkr, @benbalter Let me know if this is a good solution. Looks like it worked for me.