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.

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

Bring watch back #415

Merged
merged 9 commits into from
Sep 24, 2017
Merged

Bring watch back #415

merged 9 commits into from
Sep 24, 2017

Conversation

mertkahyaoglu
Copy link
Member

@mertkahyaoglu mertkahyaoglu commented Jul 23, 2017

Copy-pasting from this comment;

I'm trying to bring the watch back for Jekyll Admin and I think I found something to fix the race condition or (decrease the possibility to happen).

I discovered the race condition when I was trying to rename documents several times. I believe the reason is that we call site.process twice after a rename call. One for deleting a document (delete_file) and one for writing it to disk (write_file).

After I removed the site.process call for deleting and just calling site.read after writing to disk, the race condition appears to be fixed. I'm testing this almost 💯 times and I haven't been able to get an error yet.

I created a short version of delete_file method called delete_file_without_process which does not call site.process and use it for renaming purpose.

@ashmaroli
Copy link
Member

@mertkahyaoglu I tested this locally.. and..
Great news!! I did not encounter a single race error over numerous random requests..

🎉 Looks like, you've solved the problem..!! 👍

@ashmaroli
Copy link
Member

ashmaroli commented Jul 25, 2017

The not-so-good-news:
Though race condition appears fixed, some other bugs have cropped up.

Some other new bugs this will introduce

  • When you delete a Page via PageEdit view, the file is still visible in the Pages view
  • In PageEdit view, change path to a deeper loc. (eg. foo.md => bar/foo.md). Save. (Success, but the path field still has the lingering trailing bar/). Now, change the body field and save. (the file has moved to a deeper location. (bar/foo.md => bar/bar/foo.md) because of the lingering artifact in the path field

@mertkahyaoglu
Copy link
Member Author

@ashmaroli Thanks for testing. Happy to hear that 🎉

I will look into those issues.

@mertkahyaoglu
Copy link
Member Author

@ashmaroli b63f724 should fix the first issue. Second one is not related with this issue. We already have this on master. Going to create another PR for that.

@ashmaroli
Copy link
Member

Great! I'll checkout the changes and see if things work out at my end, sometime this weekend.. 👍

@ashmaroli
Copy link
Member

I updated my local branch with the latest commits........... and it looks like the Race condition is back again.. Encountered more frequently in fact, when I created a new Page by just entering the title and also when I renamed the new page..

Yep, Data files are still immune to the Race bug..

@mertkahyaoglu
Copy link
Member Author

I updated my local branch with the latest commits

You mean b63f724 ? It has nothing to do with creating and renaming files. It just reverts to site.process when deleting.

@ashmaroli
Copy link
Member

You mean b63f724 ?

I meant all the commits collectively. i.e. the state after running git pull upstream race

I wouldn't let my comment be a blocker, though. If the Race Condition doesn't occur at all for you, it probably won't affect the vast majority of the end-users.

@mertkahyaoglu
Copy link
Member Author

mertkahyaoglu commented Aug 16, 2017

Not a blocker really. If you encountered it more frequently, that means my solution does not solve the problem and I need to change my approach.

What I don't understand is in the previous comments you said the problem was fixed and after an unrelated commit, the problem came back again. I need to investigate whether that commit is really unrelated.

@ashmaroli
Copy link
Member

I don't understand it either.
I didn't check again today. But from what I remember, I was able to reproduce the RC-free state when I said it was fixed and had encountered numerous RC errors when I commented days later.

Not a blocker really

I mean do not stall this because I said I encountered bugs..

@mertkahyaoglu
Copy link
Member Author

mertkahyaoglu commented Sep 23, 2017

@ashmaroli What is this Missing magic comment # frozen_string_literal: true offense that rubocop is yelling about? I'm not seeing this on my local machine.

@ashmaroli
Copy link
Member

I'm not seeing this on my local machine.

That's probably because you're still running an older version of Jekyll locally while Travis here is running the latest v3.6.0.. You should start seeing those after you run bundle update locally..

Now regarding what they're..
Jekyll recently started enforcing frozen-strings in their codebase. Frozen strings are immutable and have a performance advantage over mutable strings..

I've not looked into its application in jekyll-admin yet.. so I suggest you disable the Rubocop check for that now by adding the following to .rubocop.yml in this repo:

Style/FrozenStringLiteralComment:
  Enabled: false

We can tackle that in a separate PR..

I suggest you add that to the master directly so that its applicable across all other PRs..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants