Skip to content

On 404, send a meta refresh header#262

Open
mgsloan wants to merge 1 commit intotapio:masterfrom
mgsloan:404-refresh
Open

On 404, send a meta refresh header#262
mgsloan wants to merge 1 commit intotapio:masterfrom
mgsloan:404-refresh

Conversation

@mgsloan
Copy link
Copy Markdown

@mgsloan mgsloan commented Jun 10, 2018

Workaround for 404s causing stuckness

With the static site generator I'm using, other paths get changed before the html of the page I'm viewing. This would cause the page to reload, but before the html file has been written. The html file is deleted at the beginning of site regeneration, so this would cause a 404 error. Since the 404 does not have the change watching script, the browser would get stuck and no longer show a live preview.

I experimented with sending the 404 with the change watching script. However, this often does not work properly. What would happen is that the file would get created with size 0, and would be read before it is populated. This 0 size file would then get served, and this would be another stuckness state.

This is a hacky solution, and I am not very familiar with node. It works well enough for my usecase, though, so I figured I would open a PR.

Race condition..

Unfortunately, serving the change watching script when the file is 0 size is not sufficient. This is because there is a race condition where file changes are ignored during reload.

It looks to me like the implementation of live-server does not have good guarantees of promptly showing the most recent version. In other words, it is straightforward to trigger a scenario where the browser is viewing an old version of the code. Here's what happens:

  1. A change occurs, notifying the client to reload
  2. The client reloads
  3. The user makes a change
  4. The reload script begins watching for changes again

Between (1) and (4), there is a gap where the client is not made aware of changes, and so the user's change in (3) will be ignored. My suggestion would be to either add incrementing counters or timestamps to the protocol. On load of the page, the client should immediately reload if it is already out of date.

@mgsloan
Copy link
Copy Markdown
Author

mgsloan commented Jun 10, 2018

Also, it looks to me like the content-length value computed in inject is wrong, because it does not take into account the length of the tag that is being substituted.

I can open issues for these things, if you would prefer that to feedback inline in PR

@mgsloan
Copy link
Copy Markdown
Author

mgsloan commented Jun 10, 2018

Another potential solution might be to avoid injection entirely, and serve some HTML that has a fullscreen iframe which will load the content. Then the outer html can have a persistent websocket that does not disconnect, and can cause the iframe to refresh.

@mgsloan
Copy link
Copy Markdown
Author

mgsloan commented Jun 10, 2018

FWIW I have decided to switch to https://www.npmjs.com/package/livereload - using a chrome extension is a more reliable way of subscribing to changes - in lieu of a more complicated protocol. Not trying to be snarky! live-server seems pretty decent.

@sanji-programmer
Copy link
Copy Markdown

@mgsloan I tried to reproduce the race unsuccessfully. I set up a small project:

https://github.com/sanji-programmer/live-server-potential-race

I used a tool I've worked with to test different schedules of the event loop, but the race you described does not show up. Could you take a look if the test case in the project is what you meant.

@mgsloan
Copy link
Copy Markdown
Author

mgsloan commented Jul 12, 2019

@sanji-programmer Hey, I really appreciate that you are digging into this. The context of my issue was https://github.com/mgsloan/mgsloan-site , where the static site generator output directory was being deleted before being recreated. The directory was being served by python3 -m http.server, and livereload was also running.

The deletion of the directory would trigger live reload, which caused a browser reload before the directory got repopulated. This would result in a 404. The race is that livereload does not cause a reload once the directory gets repopulated. I haven't dug deeply into it, but I'm thinking my proposed PR here may not be the proper solution, instead just a patch for this case.

Thanks!
-Michael

@sanji-programmer
Copy link
Copy Markdown

@mgsloan I just added two tests:

These tests can reproduce the issue with the 404 error. For trivial runs of these tests, your fix works.

Yet, it seems that some race remains. Only changing the file (https://github.com/sanji-programmer/live-server-potential-race/blob/master/race-test.js) and using the tool I've worked was not enough to reveal the race.
Nevertheless, for the tests that remove/recreate file/directory, there are some callback interleaving in which the last updated is not loaded (even with your fix). I could go as far as to notice that it is some callback related to Chokidar. If some maintainer is interested, we can go in detail with our tool's outputs.

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