Reload templates when they have been changed on disk #537

jebrosen · 2018-01-13T20:20:24Z

Resolves #163.

There are a few general aspects I would like to get feedback on:

The dependency on notify in Cargo.toml. it would be nice if this is enabled only when both debug_assertions and feature="templates" are active, i.e. target.'cfg(all(debug_assertions, feature="templates"))'.dependencies. In its current form, notify is a dependency for all debug builds but remains unused. I will continue to search for a solution to this, but it seems cargo does not support all the cfg() options that rust does, at least not in some places.
- Side note: what about making this a separate feature, instead of being based on build type? That would make this bullet point obsolete, and allow consumers to control template reloading independently of the build type.
- EDIT (2018-04-15): Discussions on IRC have led to keeping notify as a dependency in all debug builds, and also adding a new default feature (perhaps "template_reload"). The template reload behavior would now happen in cfg(all(debug_assertions, feature="template_reload")).
The mutex workaround in custom(). The on_attach callback is Fn(), so it must use a workaround such as interior mutability and Option.take() in order to move f out of its environment. The interior mutability must also be thread-safe due to the Send + Sync + 'static bounds, thus the choice of Mutex. Attach and Launch fairings shouldn't need to be Send + Sync #522 would make this workaround unnecessary, but that issue is nowhere near trivial to fix and might necessitate some amount of change to the entire Fairings system.
- EDIT (2018-04-12): Changed approach by using a new Fairing type, so this point is no longer relevant.
~~The ContextManager wrapper. Current consumers of Context assume they can access the fields of Context (that is, they have a Deref<Target=Context>, for example RwLockReadGuard<Context>).~~
This is a difficult feature to write automated tests for, and likely unreliable. Not all filesystems on all platforms are guaranteed to have reliable filesystem notification, and failing a CI build because of something nondeterministic like change notification timing would be unfortunate.
- EDIT (2018-07-10): There is a test now, and it strives to be reliable on CI. Hopefully it actually is.
User-facing and internal documentation.

Mononofu · 2018-01-15T22:13:23Z

Looking forward to this!

In the meantime, pseudo code for one possible way of testing in the face of unreliable change notification, if you don't want to use a mock:

initial = fetch_url()

timeout = now() + 5s
modify_template()
while fetch_url() == initial && now() < timeout:
  modify_template()
  sleep(10ms)

expect_ne(initial, fetch_url())

This test should pass as long as change notifications fire eventually.

SergioBenitez · 2018-04-08T23:28:28Z

contrib/src/templates/mod.rs

@@ -140,6 +143,37 @@ pub struct TemplateInfo {
    data_type: ContentType
 }

+struct ManagedContext(


Add a docstring explaining what's going on with this structure: what it's used for, why it exists, and why it looks this way.

SergioBenitez · 2018-04-08T23:29:47Z

contrib/src/templates/mod.rs

@@ -203,7 +237,7 @@ impl Template {
    ///     # ;
    /// }
    /// ```
-    pub fn custom<F>(f: F) -> impl Fairing where F: Fn(&mut Engines) + Send + Sync + 'static {
+    pub fn custom<F>(f: F) -> impl Fairing where F: Fn(&mut Engines) + Copy + Send + Sync + 'static {


I'm really not sure how I feel about this Copy restriction here. It seems very restrictive.

Will revert to the previous Mutex<Option<>> hack

SergioBenitez · 2018-04-09T00:01:33Z

contrib/src/templates/mod.rs

@@ -140,6 +143,37 @@ pub struct TemplateInfo {
    data_type: ContentType
 }

+struct ManagedContext(


I think I'd rather see:

#[cfg(not(debug_assertions))] mod context { struct ManagedContext(Context); impl ManagedContext { fn new(ctxt: Context) -> ManagedContext { ManagedContext(ctxt) } fn get(&self) -> impl Deref<Target=Context> { &self.0 } } } #[cfg(debug_assertions)] mod context { use std::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard}; struct ManagedContext(RwLock<Context>); impl ManagedContext { fn new(ctxt: Context) -> ManagedContext { ManagedContext(RwLock::new(ctxt)) } fn get(&self) -> impl Deref<Target=Context> { self.0.read().unwrap() } fn get_mut(&self) -> impl DerefMut<Target=Context> { self.0.write().unwrap() } } }

Note the use of impl Deref.

I don't like that get_mut is only included in the debug_assertions case. I suppose it's fine, but it would be nice if these were symmetric.

get_mut can only even exist because of the RwLock in debug_assertions; the structure is asymmetric because the release build does not need to write to Context and can keep the read-only form

SergioBenitez · 2018-04-09T00:05:28Z

contrib/src/templates/mod.rs

+            };
+
+            #[cfg(debug_assertions)]
+            let rocket = rocket.attach(AdHoc::on_request(|req, _| {


I think we're better off creating a custom Fairing implementation at this point as opposed to using AdHoc. This is really confusing. It took me a while to realize that this is what's actually going to reload the templates.

I think we want to create a new file, fairing.rs or something, that contains ManagedContext and some new type, TemplateFairing, for which we implement Fairing for. This time it will have both on_attach and on_request handlers. The custom method should then get really really small, probably just: TemplateFairing::new().

SergioBenitez · 2018-04-09T00:06:02Z

contrib/src/templates/mod.rs

+
+            #[cfg(debug_assertions)]
+            let rocket = rocket.attach(AdHoc::on_request(|req, _| {
+                let mc = req.guard::<State<ManagedContext>>().succeeded().expect("context wrapper");


No panics here! Log an internal error and proceed normally.

SergioBenitez · 2018-04-09T00:06:52Z

contrib/src/templates/mod.rs

@@ -315,7 +355,12 @@ impl Template {
            Status::InternalServerError
        })?;

-        let string = ctxt.engines.render(name, &info, value).ok_or_else(|| {
+        let engines = ctxt.engines.as_ref().ok_or_else(|| {


See my other comment about this being an Option.

SergioBenitez · 2018-04-09T00:07:11Z

contrib/src/templates/mod.rs

            error_!("Uninitialized template context: missing fairing.");
            info_!("To use templates, you must attach `Template::fairing()`.");
            info_!("See the `Template` documentation for more information.");
            Status::InternalServerError
        })?;
+        let ctxt = mc.get();


Just add the .get() to the thing above.

I will see what happens with other refactors and check this again, but I believe this was done because ctxt borrows from mc, so the expression assigned to mc can't be a temporary.

SergioBenitez · 2018-04-09T00:09:48Z

contrib/src/templates/watch.rs

+use std::sync::Mutex;
+use std::time::Duration;
+
+pub struct TemplateWatcher {


Do we really need this extra layer of indirection?

By extra layer do you mean the watch.rs file, or the entire structure? The motivation for this was to keep the notify dependency relatively isolated in the code.

These fields and/or behavior are small enough they could maybe move into the debug-only version of mod context in fairing.rs.

Yeah, that's what I'm thinking as well. Play around with it a bit. If we don't need the indirection, and it doesn't make code significantly cleaner, let's get rid of it.

SergioBenitez · 2018-04-09T00:10:10Z

contrib/src/templates/context.rs

-    /// Mapping from template name to its information.
-    pub engines: Engines
+    /// Loaded template engines, or None if there was a failure during a reload
+    pub engines: Option<Engines>,


Why is this an Option? If there was a failure, we should keep using the old engines.

jebrosen · 2018-04-13T01:35:51Z

contrib/src/templates/context.rs

-    pub fn initialize(root: PathBuf) -> Option<Context> {
+    pub fn initialize(
+        root: PathBuf,
+        customize_callback: &(Fn(&mut Engines) + Send + Sync + 'static),


Would be cleaner to remove the Send+Sync+'static bounds here, it's only at the level of the Fairing that these bounds are necessary

jebrosen · 2018-04-13T01:36:39Z

contrib/src/templates/fairing.rs

+    fn info(&self) -> Info {
+        Info {
+            name: "Templates",
+            kind: Kind::Attach | Kind::Request,


There is probably a speed boost to be gained by only specifying Kind::Request in debug builds, where it is actually used.

jebrosen · 2018-04-13T01:39:49Z

contrib/src/templates/watch.rs

+use std::sync::Mutex;
+use std::time::Duration;
+
+pub struct TemplateWatcher {


These fields and/or behavior are small enough they could maybe move into the debug-only version of mod context in fairing.rs.

SergioBenitez

This is looking good!

Have you investigated further whether we can use Cargo features in conjunction with cfgs to allow users to disable template reloading if they want but enable by default (and only) on debug builds? IE, so that we include template reloading only when the program is compiled in debug mode with the template_reloading feature (or some other name) enabled?

SergioBenitez · 2018-04-12T04:02:08Z

contrib/src/templates/fairing.rs

+    }
+
+    fn on_request(&self, req: &mut Request, _data: &Data) {
+        #[cfg(debug_assertions)]


Can't we put this on the fn itself?

SergioBenitez · 2018-04-13T17:23:31Z

contrib/src/templates/context.rs

@@ -45,7 +48,8 @@ impl Context {
            }
        }

-        Engines::init(&templates).map(|engines| {
+        Engines::init(&templates).map(|mut engines| {
+            customize_callback(&mut engines);


We should call the user's function after initialization. That way, we don't have to pass it in to initialize at all, and Context can stay ignorant that it even exists.

SergioBenitez · 2018-04-13T17:24:29Z

contrib/src/templates/fairing.rs

+    impl ContextManager {
+        pub fn new(ctxt: Context) -> ContextManager {
+            let root = ctxt.root.clone();
+            ContextManager {


I think you can reorder this so that you don't need the let binding:

ContextManager { watcher: TemplateWather::new(ctxt.root.clone()), context: RwLock::new(ctxt) }

SergioBenitez · 2018-04-13T17:25:05Z

contrib/src/templates/fairing.rs

+            if self.watcher.as_ref().map(TemplateWatcher::needs_reload).unwrap_or(false) {
+                warn!("Change detected, reloading templates");
+                let mut ctxt = self.get_mut();
+                match Context::initialize(ctxt.root.clone(), custom_callback) {


Maybe an if let will work better here?

SergioBenitez · 2018-04-13T17:25:36Z

contrib/src/templates/fairing.rs

+pub use self::context::ContextManager;
+
+pub struct TemplateFairing {
+    custom_callback: Box<Fn(&mut Engines) + Send + Sync + 'static>,


You should create a type alias for the function type instead of using the long type everywhere.

What should this be named? TemplateCustomizationCallback? Or maybe just CustomizationCallback?

Maybe just CustomEngineFn? Something ending in Fn would be good.

This is all that changes, is it still worth it?

diff --git a/contrib/src/templates/fairing.rs b/contrib/src/templates/fairing.rs index ab15dd3..74c6e12 100644 --- a/contrib/src/templates/fairing.rs +++ b/contrib/src/templates/fairing.rs @@ -104,12 +104,14 @@ mod context { pub use self::context::ContextManager; +pub type CustomEngineFn = Fn(&mut Engines) + Send + Sync + 'static; + pub struct TemplateFairing { - custom_callback: Box<Fn(&mut Engines) + Send + Sync + 'static>, + custom_callback: Box<CustomEngineFn>, } impl TemplateFairing { - pub fn new(custom_callback: Box<Fn(&mut Engines) + Send + Sync + 'static>) -> TemplateFairing + pub fn new(custom_callback: Box<CustomEngineFn>) -> TemplateFairing { TemplateFairing { custom_callback } }

The only other place all the bounds show up is in a trait bound, where the type alias can't be used.

SergioBenitez · 2018-04-13T17:27:55Z

contrib/src/templates/fairing.rs

+        };
+
+        match Context::initialize(template_root.clone(), &*self.custom_callback) {
+            Some(ctxt) => { Ok(rocket.manage(ContextManager::new(ctxt))) }


As in the previous comment, we don't need to pass the function to the Context initialization at all. Just call it after the context returns. Better code, and nicer separation of concerns to boot.

SergioBenitez · 2018-04-13T17:28:14Z

contrib/src/templates/fairing.rs

+    }
+
+    fn on_request(&self, _req: &mut Request, _data: &Data) {
+        #[cfg(debug_assertions)]


Use this on the fn. Not need to create a new block here.

SergioBenitez · 2018-04-13T17:28:35Z

contrib/src/templates/fairing.rs

+        #[cfg(debug_assertions)]
+        {
+            use rocket::State;
+            let cm = _req.guard::<State<ContextManager>>().unwrap();


Rename the argument to req since it's being used.

SergioBenitez · 2018-04-13T17:28:56Z

contrib/src/templates/mod.rs

 mod context;
+#[cfg(debug_assertions)] mod watch;


Move this to line 7, after the rest of the cfg-dependent imports.

SergioBenitez · 2018-04-13T17:29:47Z

contrib/src/templates/watch.rs

+use std::sync::Mutex;
+use std::time::Duration;
+
+pub struct TemplateWatcher {


Yeah, that's what I'm thinking as well. Play around with it a bit. If we don't need the indirection, and it doesn't make code significantly cleaner, let's get rid of it.

jebrosen · 2018-04-13T18:39:09Z

Thanks! Addressed most of those comments.

I will check again on the cfg situation -- the problem I encountered before was that Cargo.toml didn't seem to support the same cfg flags that rustc does.

As for the TemplateWatcher, I will try moving that into ContextManager and see how it changes things.

SergioBenitez

I think we're really close now!

About the type alias: it's unfortunate that what we really need is a trait alias. An RFC for trait aliases has actually been accepted (rust-lang/rfcs#1733), but according to the tracking issue, it is unimplemented (rust-lang/rust#41517).

We can use a hack for now:

trait AliasName: TraitsToAlias { }
impl<T: TraitsToAlias> AliasName for T { }

Then you can use AliasName anywhere you would have used TraitsToAlias

SergioBenitez · 2018-04-18T16:05:41Z

contrib/src/templates/fairing.rs

+        /// The current template context, inside an RwLock so it can be updated
+        context: RwLock<Context>,
+        /// A filesystem watcher. Unused in the code after creation, but must be kept alive
+        _watcher: Option<RecommendedWatcher>,


Copying from IRC:

[09:01:45] ~Sergio: occultus: What's the deal with _watcher? [09:01:54] ~Sergio: It seems really odd that we create a thing and never use it, but it has to be kept around. [09:04:10] ~Sergio: occultus: Without reading any additional documentation, my guess is that it has to be kept around for the `recv_queue` to have something to read from. If that's the case, then we should group the `RecommendedWatcher` and the `rx` inside the `Option`. No use in calling `recv()` on an `rx` that won't ever have anything. [09:05:13] ~Sergio: IE, we should change `_watcher: Option<RecommendedWatcher>` to `watcher: Option<(RecommendedWatcher, Mutex<Receiver<DebouncedEvent>>)>`, or something to that affect.

SergioBenitez · 2018-04-18T16:05:56Z

contrib/src/templates/fairing.rs

+        pub fn new(ctxt: Context) -> ContextManager {
+            let (tx, rx) = channel();
+
+            let _watcher = if let Ok(mut watcher) = watcher(tx, Duration::from_secs(1)) {


Can we use and_then and map_err to clean this up?

They can be used (somewhat awkwardly), but they don't appear to clean it up. I'll take another stab at it later though.

SergioBenitez · 2018-04-18T16:06:07Z

contrib/src/templates/fairing.rs

+                if watcher.watch(ctxt.root.clone(), RecursiveMode::Recursive).is_ok() {
+                    Some(watcher)
+                } else {
+                    warn!("Could not monitor the templates directory for changes. Live template reload will be unavailable");


This line and the other warn! are too long.

SergioBenitez · 2018-04-18T16:06:15Z

contrib/src/templates/fairing.rs

+                    custom_callback(&mut new_ctxt.engines);
+                    *ctxt = new_ctxt;
+                } else {
+                    warn!("An error occurred while reloading templates. The previous templates will remain active.");


This line is too long.

SergioBenitez · 2018-04-18T16:08:35Z

contrib/src/templates/fairing.rs

+
+impl TemplateFairing {
+    pub fn new(custom_callback: Box<Fn(&mut Engines) + Send + Sync + 'static>) -> TemplateFairing
+    {


This should either be at the end of the previous line, or we should split the arguments each into their own line.

SergioBenitez · 2018-04-18T16:09:03Z

contrib/src/templates/fairing.rs

+            }
+        };
+
+        match Context::initialize(template_root.clone()) {


SergioBenitez · 2018-04-18T16:09:28Z

contrib/src/templates/fairing.rs

+    #[cfg(debug_assertions)]
+    fn on_request(&self, req: &mut Request, _data: &Data) {
+        use rocket::State;
+        let cm = req.guard::<State<ContextManager>>().unwrap();


Do rocket::State or ::rocket::State here instead of using rocket::State at the top.

jebrosen · 2018-04-19T03:12:23Z

The trait alias workaround didn't work as easily as I expected, with some errors about missing the Output associated type of FnOnce, which I attempted to solve a few ways demonstrated here:

https://play.rust-lang.org/?gist=cd5cdbf4680729afb0c8a8feb724bd37&version=nightly

EDIT: I found rust-lang/rust#24010 , which is 3 (!) years old and appears to be what I ran into.

jebrosen · 2018-04-19T03:16:27Z

(adapted from IRC) I tried adding template_reload as a feature that is on by default, but I realized most of the docs use default_features = false on rocket_contrib and I suspect that isn't an uncommon situation. Alternatively, an opt-in feature called no_template_reload would permit the desired behavior. Thoughts?

jebrosen · 2018-04-29T00:38:02Z

Sorry about going off the radar for a while. In summary, I think this is what's still unaddressed as far as design:

Trying to not repeat Fn(&mut Engines) + Send + Sync + 'static by using an alias. A trait alias ends up being repetitive in other ways (<Output=()>) because of Error extending Fn trait then using it as a trait object: the value of the associated type `Output must be specified rust-lang/rust#24010 AKA Incorrect "associated type must be specified" for trait objects rust-lang/rust#23856 .
Gating by a feature, e.g. "template_reload". The main question I have about this is whether it makes sense to be a default-on feature, when several docs for rocket_contrib and therefore many users are likely to use default_features=false anyway.
Tests. These should, at a minimum, verify that writing to a file in the templates directory triggers a template reload in the appropriate profile/feature combinations. I haven't thought much about testing the different error conditions.

I would like feedback/a decision on the first two points, and I was hoping to write some tests within the next few days.

SergioBenitez · 2018-07-02T20:45:59Z

@jebrosen Looks like the test failed in debug mode.

jebrosen · 2018-07-02T21:07:33Z

Local test output:

GET /:
Warning: Change detected, reloading templates
    => Error: No matching routes for GET /.
    => Warning: Responding with 404 Not Found catcher.

Looks like the change is not picked up on the CI. I will switch from using notify's DebouncedEvent to RawEvent in case it is a timing issue.

jebrosen · 2018-07-04T16:00:05Z

So far, that's one CI build that failed with raw_watcher, and one CI build that succeeded with raw_watcher. I haven't had the test fail locally (yet).

SergioBenitez · 2018-07-08T02:06:13Z

Looks like a race with the test. Perhaps try adding a flush() or sink() before checking that the new template was picked up. Additionally, or instead, you may want to add a short, say, 100ms delay between writing and checking for a reload.

jebrosen · 2018-07-08T02:16:35Z

Ah. I've looked over the different methods available on File and their implementations and I will try sync_all for this. I had assumed that Drop would do the right thing, but I forgot that an immediate sync isn't guaranteed. In the meantime, do you have any thoughts on my earlier comments about the trait alias and the feature declaration?

SergioBenitez · 2018-07-08T22:22:39Z

@jebrosen I don't think the repetition is a big deal. I think the best we can do is:

type CallBack = Box<Fn(&mut Engines) + Send + Sync + 'static>;

But even that may not be worth it. I wouldn't worry about it too much. I think if we can get the tests to work reliably (i.e, 100% of the time, otherwise it will make life much harder down the road), then we can proceed to final polish.

SergioBenitez · 2018-07-14T20:48:38Z

contrib/lib/tests/templates.rs

+            // write a change to the file
+            write_file(&reload_path, NEW_TEXT);
+
+            let mut attempts = 0;


I'd prefer if it was written the following way (I did 6 just because 1.5s):

for i in 0..6 { // dispatch any request to trigger a template reload client.get("/").dispatch(); // if the new content is correct, we are done let new_rendered = Template::show(client.rocket(), RELOAD_TEMPLATE, ()); if new_rendered == Some(NEW_TEXT.into()) { remove_file(reload_path).expect("delete template file"); return; } thread::sleep(Duration::from_millis(250)); } panic!("failed to reload template in 1.5s");

SergioBenitez

I'm pretty happy with this. After these mostly minor changes, we'll need some docs in the main template module as well as in the guide explaining how this works. In particular, those docs should make it clear when templates are reloaded (debug mode only, when a file write occurs, on the next request) and the performance implications.

SergioBenitez · 2018-07-21T05:58:46Z

contrib/lib/src/templates/fairing.rs

+                }
+
+                if changed {
+                    warn!("Change detected, reloading templates");


Should probably be an info!("Change detected: reloading templates.").

Should this maybe be info_! as well? I have to admit I'm not sure when to use the indented forms and when not to.

SergioBenitez · 2018-07-21T05:58:57Z

contrib/lib/src/templates/fairing.rs

+                    Some((watcher, Mutex::new(rx)))
+                } else {
+                    warn!("Could not monitor the templates directory for changes.");
+                    warn!("Live template reload will be unavailable");


Should be warn_!.

SergioBenitez · 2018-07-21T05:59:02Z

contrib/lib/src/templates/fairing.rs

+                }
+            } else {
+                warn!("Could not instantiate a filesystem watcher.");
+                warn!("Live template reload will be unavailable");


Should be warn_!.

SergioBenitez · 2018-07-21T05:59:34Z

contrib/lib/src/templates/fairing.rs

+pub use self::context::ContextManager;
+
+pub struct TemplateFairing {
+    custom_callback: Box<Fn(&mut Engines) + Send + Sync + 'static>,


Can we add a docstring to say what this is?

I added one to the type itself. I'm not sure if you meant the fairing or the callback.

SergioBenitez · 2018-07-21T05:59:44Z

contrib/lib/src/templates/fairing.rs

+            self.context.write().unwrap()
+        }
+
+        pub fn reload_if_needed<F: Fn(&mut Engines)>(&self, custom_callback: F) {


Can we add a docstring to this method?

SergioBenitez · 2018-07-21T06:07:08Z

contrib/lib/src/templates/mod.rs

            error_!("Uninitialized template context: missing fairing.");
            info_!("To use templates, you must attach `Template::fairing()`.");
            info_!("See the `Template` documentation for more information.");
            Status::InternalServerError
        })?;
+        let ctxt = cm.get();

        let (render, content_type) = self.finalize(&ctxt)?;


Presumably we can't call cm.get() here directly because of some borrow checking issues?

Yes, and I think I may have finally figured out why. The State type implements Deref, whose lifetime is tied to the State object itself. Using .inner() returns a longer lifetime and "escapes" out of the temporary State value.

SergioBenitez · 2018-07-21T06:07:25Z

contrib/lib/tests/templates.rs

@@ -116,5 +116,57 @@ mod templates_tests {
            let response = client.get("/tera/test").dispatch();
            assert_eq!(response.status(), Status::NotFound);
        }
+
+        #[cfg(debug_assertions)]
+        #[test]


Can we flip these two attributes so that longer one is at the bottom?

SergioBenitez · 2018-07-21T06:07:40Z

contrib/lib/tests/templates.rs

+        #[cfg(debug_assertions)]
+        #[test]
+        fn test_template_reload() {
+            use rocket::local::Client;


Do the std imports before the rocket imports.

SergioBenitez · 2018-07-21T06:08:20Z

contrib/lib/tests/templates.rs

+                file.sync_all().expect("sync file");
+            }
+
+            const RELOAD_TEMPLATE: &'static str = "hbs/reload";


consts should come before everything else. Note that we don't need the 'static since that's implied.

SergioBenitez · 2018-07-21T06:09:30Z

contrib/lib/tests/templates.rs

+                "tests/templates/hbs/reload.txt.hbs"
+            );
+
+            write_file(&reload_path, INITIAL_TEXT);


Add a comment above this. I suppose it's // set up the template with the initial text _before_ initializing the Rocket instance so that it's picked up on the initial load.

SergioBenitez · 2018-07-21T19:41:28Z

contrib/lib/src/templates/fairing.rs

+                if watcher.watch(ctxt.root.clone(), RecursiveMode::Recursive).is_ok() {
+                    Some((watcher, Mutex::new(rx)))
+                } else {
+                    warn_!("Could not monitor the templates directory for changes.");


Ah, so, the _ just adds a level of indentation to allow for grouping messages. Here, we'd want the one on the top not to use the _ so we get this:

Warning: Could not monitor the templates directory for changes. => Live template reload will be unavailable.

In other words, the _ is for cosmetic/grouping purposes.

SergioBenitez

This is excellent! Aside from these tiny changes (style + 1 docstring), all we need to merge is module level and guide levels. Whew!

SergioBenitez · 2018-07-21T19:42:08Z

contrib/lib/src/templates/fairing.rs

+            self.context.write().unwrap()
+        }
+
+        /// Checks whether any template files have changed on disk.  If


Word wrap at 80 columns.

SergioBenitez · 2018-07-21T19:42:22Z

contrib/lib/src/templates/fairing.rs

+
+pub use self::context::ContextManager;
+
+/// The TemplateFairing initializes the template system on attach,


Word wrap at 80 cols.

SergioBenitez · 2018-07-21T19:42:49Z

contrib/lib/src/templates/fairing.rs

+/// In debug mode, the fairing checks for modifications to templates
+/// before every request and reloads them if necessary.
+pub struct TemplateFairing {
+    pub(crate) custom_callback: Box<Fn(&mut Engines) + Send + Sync + 'static>,


Can we get a short docstring on custom_callback as well?

…ilure during reloading

…emplateWatcher into the debug version of ManagedContext

hopefully helps catch reload conditions more quickly, and notify won't do any of its extra processing (that we ignore anyway)

… systems for changes to be detected properly

…t can break other concurrent tests

…ed file

SergioBenitez · 2018-08-11T06:49:52Z

Merged in 491b04c. Whew! So happy to finally land this. Excellent job as always, @jebrosen!

jebrosen changed the title ~~Template reload~~ Reload templates when they have been changed on disk Jan 13, 2018

SergioBenitez requested changes Apr 9, 2018

View reviewed changes

jebrosen force-pushed the template_reload branch from 8850f35 to 4e6fe24 Compare April 9, 2018 03:12

jebrosen commented Apr 13, 2018

View reviewed changes

SergioBenitez requested changes Apr 13, 2018

View reviewed changes

jebrosen force-pushed the template_reload branch 2 times, most recently from 0687c51 to e1a43f8 Compare April 18, 2018 15:30

SergioBenitez requested changes Apr 18, 2018

View reviewed changes

jebrosen force-pushed the template_reload branch from bfc5ec3 to d02d305 Compare May 7, 2018 16:34

jebrosen force-pushed the template_reload branch from d02d305 to c55c509 Compare June 3, 2018 20:14

jebrosen force-pushed the template_reload branch from c55c509 to 1af3416 Compare June 29, 2018 19:35

jebrosen force-pushed the template_reload branch from f3e8950 to 7b51fe6 Compare July 4, 2018 08:40

jebrosen force-pushed the template_reload branch 2 times, most recently from f5a60c0 to ddc37e7 Compare July 12, 2018 00:56

SergioBenitez reviewed Jul 14, 2018

View reviewed changes

SergioBenitez requested changes Jul 21, 2018

View reviewed changes

jebrosen force-pushed the template_reload branch from 5b19181 to f5a2c88 Compare July 21, 2018 15:37

SergioBenitez reviewed Jul 21, 2018

View reviewed changes

SergioBenitez requested changes Jul 21, 2018

View reviewed changes

jebrosen added 23 commits August 9, 2018 13:40

template reload: Keep previous templates and engines if there is a fa…

5771e94

…ilure during reloading

mostly revert Context: move the callback into the fairing, move the T…

10b0c33

…emplateWatcher into the debug version of ManagedContext

rename ManagedContext to ContextManager

91a4e38

remove Send+Sync+'static bounds where they are not needed

1a517cc

register the on_request part of TemplateFairing only in debug_assertions

ef6159d

refactor based on feedback

c301c13

move TemplateWatcher functionality directly into ManagedContext

8b54c86

merge watcher and corresponding receive queue into same Option field

5340c5a

minor style fixes

4d3a0ff

split the long warning lines

9807844

template reload: simplest test of a single template reload cycle

970056c

switch watcher to raw_watcher

d94c4ba

hopefully helps catch reload conditions more quickly, and notify won't do any of its extra processing (that we ignore anyway)

explicitly sync writes to files in the test; likely necessary on some…

0f3daa1

… systems for changes to be detected properly

unbreak rwf2#676 when combined with ContextManager

8b28f80

fully qualify Data, Request to avoid unused warnings in release mode

91204ae

template_reload: attempt several times to observe filesystem changes

d37ed0f

refactor loop based on feedback. don't delete template file becuase i…

f4176df

…t can break other concurrent tests

template reload tweaks and documentation

2585897

style changes, documentation additions/updates

d75d1ec

update note about reload being disabled in release & add to guide

e13036e

include template reload test file so it does not appear as an untrack…

85f2a5d

…ed file

missing 'of'

dc34ab6

remove an unnecessary clone()

3ecc02a

jebrosen force-pushed the template_reload branch from f1c8af1 to 3ecc02a Compare August 9, 2018 19:47

SergioBenitez closed this Aug 11, 2018

jebrosen deleted the template_reload branch August 11, 2018 14:43

SergioBenitez added the pr: merged This pull request was merged manually. label Aug 17, 2018

jebrosen mentioned this pull request Aug 2, 2019

Can Template::custom() take an FnOnce? #1064

Closed

jebrosen mentioned this pull request Jun 6, 2020

Make 'notify' an optional dependency. #1328

Closed


		pub use self::context::ContextManager;

		/// The TemplateFairing initializes the template system on attach,

Reload templates when they have been changed on disk #537

Reload templates when they have been changed on disk #537

Conversation

jebrosen commented Jan 13, 2018 • edited Loading

Mononofu commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jebrosen Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jebrosen commented Apr 13, 2018

SergioBenitez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jebrosen commented Apr 19, 2018 • edited Loading

jebrosen commented Apr 19, 2018

jebrosen commented Apr 29, 2018 • edited Loading

SergioBenitez commented Jul 2, 2018

jebrosen commented Jul 2, 2018

jebrosen commented Jul 4, 2018

SergioBenitez commented Jul 8, 2018

jebrosen commented Jul 8, 2018

SergioBenitez commented Jul 8, 2018

SergioBenitez Jul 14, 2018 • edited Loading

Choose a reason for hiding this comment

SergioBenitez left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioBenitez left a comment

jebrosen commented Jan 13, 2018 •

edited

Loading

SergioBenitez Apr 9, 2018 •

edited

Loading

jebrosen Apr 9, 2018 •

edited

Loading

jebrosen commented Apr 19, 2018 •

edited

Loading

jebrosen commented Apr 29, 2018 •

edited

Loading

SergioBenitez Jul 14, 2018 •

edited

Loading

SergioBenitez left a comment •

edited

Loading