sunng87 / handlebars-iron

Handlebars middleware for Iron web framework
MIT License
119 stars 20 forks source link

Fix race condition in the watch feature #45

Closed cmbrandenburg closed 8 years ago

cmbrandenburg commented 8 years ago

This affects issue #10, possibly resolving it.

This commit fixes a race condition due to the watch object being destructed and reconstructed after each file system event. It's possible for a file system event to occur after one of these destructions but before the next construction such that a second, subsequent file system event is lost.

Here's a possible scenario:

  1. File system event A occurs.
  2. The watch object destructs.
  3. The template engine is reloaded.
  4. File system event B occurs.
  5. A new watch object is constructed.

In this scenario, the second watcher will not detect event B. If B represents a template being updated then Handlebars Iron will not pick up the change.

cmbrandenburg commented 8 years ago

This change may improve performance for real-world scenarios because there's less stuff happening per file system event. However, in some circumstances performance is worse.

For example, suppose you spam the watched directory with a lot of file system events (e.g., for i in {1..1000}; do touch foo$i; done && rm foo*). Because this fix causes Handlebars Iron to detect all file system events, the watch thread may consume more CPU. Whereas, before applying this fix, many—if not most—of the events would be undetected.

Nevertheless, correctness outweighs performance, and, in most real-world cases, developers aren't spamming their template directory.

sunng87 commented 8 years ago

Thank you @cmbrandenburg ! This original code that recreate watcher for every event is unnecessary. Your patch makes sense.

For the scenario with of massive changes in short time, I'm not sure if we can use compare the event timestamp with reload timestamp and ignore(merge) some events. Anyway we can ignore this scenario for now.