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

livereload reloads for every file that changes #2061

Closed
Stanzilla opened this issue Apr 13, 2020 · 25 comments · Fixed by #2385
Closed

livereload reloads for every file that changes #2061

Stanzilla opened this issue Apr 13, 2020 · 25 comments · Fixed by #2385

Comments

@Stanzilla
Copy link

Currently when developing a theme, having mkdocs run via serve reloads the page as often as each file changes in a theme. So if run my theme's build process, it reloads the page around 34564564564640 times or more.

@waylan
Copy link
Member

waylan commented Apr 13, 2020

Then I would suggest shutting down the dev server when you build your theme. Note that the builtin themes have no build process but are rather simple. And wow, that is a lot of files for a theme.

@Stanzilla
Copy link
Author

Yeah that is what I am doing at the moment and trying to avoid :)

@squidfunk
Copy link
Contributor

squidfunk commented Apr 13, 2020

This is indeed super annoying and shouldn't be too hard to fix by batching updates. It can easily be reproduced by checking out squidfunk/mkdocs-material, start mkdocs serve in one session and the Webpack watcher with npm start in another one. After Webpack rebuilds, mkdocs serve will reload more than 5.000 times (because of all bundled icons) for every asset that's contained within the build.

@waylan
Copy link
Member

waylan commented Apr 13, 2020

The MkDocs serve command is a light wrapper around lepture/python-livereload. When used directly, that tool provides a --wait option which causes the watcher to delay sending the reload signal by the specified number of seconds. Would adding that option to MkDocs address this? If so, a PR is welcome.

Note that as this is not needed by the core dev team, it is a low priority and not something we are likely to implement ourselves. However, I am happy to review a PR if someone else wants to do the work.

As a workaround, you can always use python-livereload directly.

@squidfunk
Copy link
Contributor

I understand that this is not a priority. However, note that there’re more than 900 forks of Material for MkDocs which all have this problem, so quite an audience.

@squidfunk
Copy link
Contributor

squidfunk commented Apr 14, 2020

I think there's something that needs to be done on that front.

Material for MkDocs now bundles 5k icons, which is what the OP is referring to. Not only will python-livereload queue up every file and reload the page, again and again, the general rebuild time has also increased. In squidfunk/mkdocs-material#1598, the OP says that a reload now takes 10.5s as opposed to 4.5s before (which also is quite slow).

Additionally, python-livereload is looking for maintainers and seems rather abandoned (the last commit was 6 months ago), so maybe this is a good time switching it for something faster and less annoying.

Again, I understand, that this is not a priority from the maintainers' perspective, but given the sheer amount of people using Material for MkDocs, which accounts for 40% or more of all MkDocs installations, it would be great, if alternatives could be evaluated.

EDIT: nvm, I misunderstood the OP in the linked issue. However, solving the queueing problem would be awesome!

@waylan
Copy link
Member

waylan commented May 5, 2020

I just created PR #2093 which adds a --wait flag to the serve command. However, I'm not perceiving any change in behavior. But then I don't have a need for the delay. If someone could try it out and report back that would be helpful. Please follow up on the PR rather than here.

@waylan
Copy link
Member

waylan commented Apr 8, 2021

I am reopening this issue as #2093 has been reverted in #2285.

Note that no one should be writing to the docs_dir during a build. A plugin would instead use a File object to point to an outside location. However, it is understandable that a theme developer may need to "build" their theme (for example render CSS from LESS, SCSS, SASS, etc) and we need some way to support that without rebuilding every time a new file is written to the theme dir by the theme's "build" process.

The issue reported here (specific to themes) is avoided by the fix in #2094 as the theme dir is no longer watched by default. However, if a theme developer is using the --watch-theme option introduced in #2094, then this issue still exists.

@waylan waylan reopened this Apr 8, 2021
@oprypin
Copy link
Member

oprypin commented Apr 8, 2021

The way to observe this issue currently, regardless of themes:

mkdocs serve
Wait for the first build to finish and open another terminal.
find . -name '*.md' -exec touch {} \;
If there are N files, you will see the site rebuild itself in a loop N times, without any interaction whatsoever.

@oprypin
Copy link
Member

oprypin commented Apr 8, 2021

Unfortunately the 'livereload' module indeed provides no way to batch multiple filesystem updates into one.
It only kinda attempts to alleviate this problem here
https://github.com/lepture/python-livereload/blob/4a8e4031b3bb1322beb5ce285fac84104b4151ce/livereload/handlers.py#L80
but that applies only if the callback's duration is very small, if it works at all (I wasn't able to observe this "ignore" behavior)

It is possible to hack around it, by adding a wrapper with a tiny (non-configurable) delay and waiting for it to produce another callback in that period, for the purpose of batching them.

Or, I found this alternative implementation (seems like the author just wanted a better rewrite of livereload), and it appears to do the right logic for batching here:
https://github.com/thanethomson/httpwatcher/blob/373bddf1af99f5cc84917b5c8d4075ce3725a2ad/httpwatcher/filesystem.py#L64-L68

@waylan
Copy link
Member

waylan commented Apr 8, 2021

Or, I found this alternative implementation (seems like the author just wanted a better rewrite of livereload), and it appears to do the right logic for batching here:
https://github.com/thanethomson/httpwatcher/blob/373bddf1af99f5cc84917b5c8d4075ce3725a2ad/httpwatcher/filesystem.py#L64-L68

Awesome! I hadn't seen that before. I would be okay with exploring switching to an alternate livereload server. The existing server has been one of the largest pain points and our requests for upstream changes often go unheard as the package has a different goal (see lepture/python-livereload#131 for example). Interestingly, the README for httpwatcher specifically mentions that it was build to support a static site generator. I also find it interesting that it uses watchdog rather than reinventing that wheel.

@oprypin
Copy link
Member

oprypin commented Apr 8, 2021

Sadly, seems abandoned :( thanethomson/httpwatcher#2
Though reviving it isn't fully out of the question

@waylan
Copy link
Member

waylan commented Apr 8, 2021

Sadly, seems abandoned

That is unfortunate.

@jleclanche
Copy link

jleclanche commented Apr 8, 2021

Maybe take a page from FastAPI's playbook and consider using uvicorn as server. It does have live reload implemented. (Those are python live reload events but i suspect it can be tweaked / hooked into)

@oprypin
Copy link
Member

oprypin commented Apr 29, 2021

I am presenting a new livereload implementation without these shortcomings.
See #2385, and I welcome people to try it out.

@charlescurley
Copy link

It would be nice to see this (or something like it) implemented. Emacs tends to use temporary files (e.g. #index.md#, below), which sometimes get caught up in the process.

[I 210507 15:08:56 handlers:92] Reload 1 waiters: /home/charles/versioned/emutosdocs/usermanual/docs/#index.md#
[I 210507 15:08:56 handlers:132] Browser Connected: http://127.0.0.1:8000/
[I 210507 15:09:01 watcher:92] Running task: <function serve.<locals>.builder at 0x7f627ea07598> (delay: None)
INFO    -  Building documentation... 
Exception in callback BaseAsyncIOLoop._handle_events(3, 1)
handle: <Handle BaseAsyncIOLoop._handle_events(3, 1)>
Traceback (most recent call last):
  File "/usr/lib/python3.7/asyncio/events.py", line 88, in _run
    self._context.run(self._callback, *self._args)
  File "/usr/lib/python3/dist-packages/tornado/platform/asyncio.py", line 122, in _handle_events
    handler_func(fileobj, events)
  File "/usr/lib/python3/dist-packages/tornado/stack_context.py", line 300, in null_wrapper
    return fn(*args, **kwargs)
  File "/usr/lib/python3/dist-packages/pyinotify.py", line 1560, in handle_read
    self.process_events()
  File "/usr/lib/python3/dist-packages/pyinotify.py", line 1275, in process_events
    self._default_proc_fun(revent)
  File "/usr/lib/python3/dist-packages/livereload/watcher.py", line 165, in inotify_event
    self.callback()
  File "/usr/lib/python3/dist-packages/livereload/handlers.py", line 66, in poll_tasks
    filepath, delay = cls.watcher.examine()
  File "/usr/lib/python3/dist-packages/livereload/watcher.py", line 93, in examine
    func()
  File "/usr/lib/python3/dist-packages/mkdocs/commands/serve.py", line 114, in builder
    build(config, live_server=live_server, dirty=dirty)
  File "/usr/lib/python3/dist-packages/mkdocs/commands/build.py", line 285, in build
    files.copy_static_files(dirty=dirty)
  File "/usr/lib/python3/dist-packages/mkdocs/structure/files.py", line 44, in copy_static_files
    file.copy_file(dirty)
  File "/usr/lib/python3/dist-packages/mkdocs/structure/files.py", line 178, in copy_file
    utils.copy_file(self.abs_src_path, self.abs_dest_path)
  File "/usr/lib/python3/dist-packages/mkdocs/utils/__init__.py", line 120, in copy_file
    shutil.copyfile(source_path, output_path)
  File "/usr/lib/python3.7/shutil.py", line 120, in copyfile
    with open(src, 'rb') as fsrc:
FileNotFoundError: [Errno 2] No such file or directory: '/home/charles/versioned/emutosdocs/usermanual/docs/#index.md#'
[I 210507 15:09:09 watcher:92] Running task: <function serve.<locals>.builder at 0x7f627ea07598> (delay: None)
INFO    -  Building documentation... 
[I 210507 15:09:09 handlers:92] Reload 1 waiters: /home/charles/versioned/emutosdocs/usermanual/docs/index.md
[I 210507 15:09:10 handlers:132] Browser Connected: http://127.0.0.1:8000/

An alternative that would solve my problem would be to ignore temporary and backup files, such as #*# and *~.

@oprypin
Copy link
Member

oprypin commented May 7, 2021

Then you're in a great position to try out #2385. Please let me know if it helps for that situation.

@charlescurley
Copy link

charlescurley commented May 8, 2021 via email

@waylan
Copy link
Member

waylan commented May 10, 2021

@charlescurley you should be able to install from @oprypin's branch directly using this command:

pip install git+https://github.com/oprypin/mkdocs.git@reload

I know I would certainly like some feedback from users who have a need for this.

@charlescurley
Copy link

charlescurley commented May 10, 2021 via email

@oprypin
Copy link
Member

oprypin commented May 10, 2021

Thanks for your report. I could reproduce this if I also configure my editor "Kate" to write to the local directory instead of a separate global one (which emacs really should be doing instead...)

I think what saved the previous implementation is that it was checking for files' modification times before concluding that the file changed.
https://github.com/lepture/python-livereload/blob/ecabebaeee2a40743f5a8216e1856c9f9150913f/livereload/watcher.py#L180

In our case I have pushed new code to rely only on "file close" events that editors always emit on save, and disregard "file modified" events which get tripped by these swap writes.

Please try this again and let me know how that goes. Now it really should be better than the old server.

Regarding ignoring certain filenames: I think it's not a clean approach, and would end up being the usual whack-a-mole...

@oprypin
Copy link
Member

oprypin commented May 11, 2021

Well I can guess that it will work for you, but I also see that this approach completely doesn't work on Mac and Windows, so nevermind...

@waylan
Copy link
Member

waylan commented May 11, 2021

@charlescurley thanks for that feedback. It is very helpful. Those of us who don't use emacs forget about its weird behavior.

However, could we please keep any discusions and feedback about the code in the PR to the PR and not here? Thanks.

@jobveldhuis
Copy link

I am currently in the middle of reviving httpwatcher and I see this has been mentioned here before. Is there any feature you need/require that is not in the httpwatcher module?

@oprypin
Copy link
Member

oprypin commented May 23, 2021

@jobveldhuis Thanks for the mention. No, I have no interest in httpwatcher, as I'm quite convinced that the implementation from #2385 basically makes it obsolete (unless you have the specific requirement of integrating with "tornado" server). httpwatcher runs on 1-second polling, with (as I realize now) most of the same flaws in terms of batching incoming events. If you're looking for a good livereload server, I can look into releasing this as a library.

waylan pushed a commit that referenced this issue May 25, 2021
This discards the dependency on 'livereload' and 'tornado'. This implementation is based on stdlib http.server and is much smaller, while not being a downgrade in any way.

This fixes #2061: multiple file changes no longer cause the site to repeatedly rebuild.
It also makes reloads much more responsive; they're "instant" rather than being on a 1-second polling interval or such.

Each HTML page gets JavaScript injected into it (like before) but now, instead of connecting to WebSocket, it requests a long-polling endpoint, which blocks until a newer version ("epoch") of the site is available - then the page knows to reload itself. After a minute without events, the endpoint will just return the unchanged epoch (to avoid timeouts), then the page requests the same endpoint again. The "downtime" in between requests is not a problem because these are not events that need to be received in real time; the endpoint can instantly report that the latest version is newer than the one the page identified itself as.

The reason to replace WebSocket isn't that it's bad or something, just that stdlib doesn't have it. But long-polling works completely fine here too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants