Fix watchdog reload worker repeatedly if there are multiple changed files

[lexhung]

When there are multiple changed files (e.g. git pull, installing new version of a package, etc.), watchdog attempts to reload workers at first detected change and repeat for every changed file.

The patch allow gathering all changes into a single restart action.

[codecov]

Codecov Report

Merging #1555 into master will increase coverage by 2.04%.
The diff coverage is 82.75%.

@@            Coverage Diff             @@
##           master    #1555      +/-   ##
==========================================
+ Coverage   92.17%   94.21%   +2.04%     
==========================================
  Files          23       22       -1     
  Lines        2312     2315       +3     
  Branches      424      426       +2     
==========================================
+ Hits         2131     2181      +50     
+ Misses        141       84      -57     
- Partials       40       50      +10     

Impacted Files	Coverage Δ
sanic/app.py	92.06% <0.00%> (-0.20%)	⬇️
sanic/reloader_helpers.py	67.32% <88.88%> (+52.58%)	⬆️
sanic/__init__.py

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c04c9a...1b64573. Read the comment docs.

[ahopkins]

Thanks for the PR.

Why not break out of the loop when need_reload is True, instead of continuing to look at every file?

[lexhung]

Why not break out of the loop when need_reload is True, instead of continuing to look at every file?

In my case, when I update a dependency package there are about 30 files being touched at the same time which trigger the app to reload 30 times continuously (breaking the loop there will only update the mtime of a single file). This patch allow all modified files' mtime to be updated before trigger reload.

It should have no big performance impact since it have to scan all files in the ordinary case when no file is modified.

[ahopkins]

I guess... it does seem like it would potentially have a negative impact though. Especially for larger projects. I am hesitant to accept a change like this that could potentially have a large impact on a number of developers in their daily workflow.

I understand your issue and concern, but I would rather find a solution that does not also impact the existing operation.

[ahopkins]

Instead, what if we make watchdog a configurable param:

app.run(watchdog=my_custom_watchdog)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

[Tronic]

I might have a better look at the auto-reloader in the coming days; its killing of existing processes seems rather broken, and this patch is probably a step in the right direction.

[Tronic]

This needs a few changes. First of all, multiprocessing.Process is used to run subprocess ... meaning a chain of two processes. After fixing that so that subprocess is used directly, it can also be terminated reliably, and we no longer need those hacks to kill all child processes.

I worked on this in the Trio branch, but in principle the changes should apply to asyncio implementation too. It appears to be working with single as well as multiple workers, as long as the master server kills its worker processes when it receives SIGTERM (dunno how Windows-compatible that is, might need another solution there).

My current revision:
https://github.com/huge-success/sanic/blob/53f088e0cf3c5584ece8f61de5082970a9514b78/sanic/reloader_helpers.py

@lexhung could you give it a little bit of polish and testing on current Sanic master?

[lexhung]

Hi @Tronic We may want to go through manual review for this to get merged.

I implemented the tests however it is quite tricky to get the patch coverage passed. These are parts that related to OS exceptions and it is not clear to me how to set that up without making the tests brittle.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

[Tronic]

@ahopkins Are you reviewing this?

[ahopkins]

Yes. I am taking a look at it. I will respond with more detailed thoughts when I am back at my workstation (beginning of the week) and can fire up a few tests on this.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

[Tronic]

Looks like this is still stuck with no review. Even if the patch isn't quite the ideal solution we might have in our minds, I believe that it is definitely an improvement and it works on my machine, so I suggest for this to be merged as well.

[ahopkins]

I am thinking this should be closed once #1827 is finished. Am I mistaken?

[Tronic]

@ahopkins #1827 is already finished but it was a bit too late & scary change for 20.3 (needs review now). Closing in favour of #1827 which contains these changes as well.