filesystem: provide kqueue-based impl of Watcher by zuercher · Pull Request #1363 · envoyproxy/envoy

zuercher · 2017-08-01T00:03:27Z

(Split out of #1348, part of fixes for #128).

This implementation watches the given file, if it exists. Otherwise it watches the directory for writes and then checks to see if the directory modification caused the file to appear.

mattklein123

Thanks very much for splitting this out. Few small comments. Generally LGTM.

mattklein123 · 2017-08-01T01:55:41Z

bazel/envoy_build_system.bzl

        linkstatic = 1,
        linkstamp = linkstamp,
-    )
+         strip_include_prefix = strip_include_prefix,


nit: indent

mattklein123 · 2017-08-01T01:56:56Z

source/common/filesystem/kqueue/watcher_impl.cc

+         reinterpret_cast<void*>(watch_fd));
+
+  if (kevent(queue_, &event, 1, NULL, 0, NULL) == -1) {
+    throw EnvoyException(


watch->fd_ will leak here when you throw and in other cases. Can you add close() to the destructor of FileWatch, then you can make overall destruction simpler.

mattklein123 · 2017-08-01T01:57:08Z

source/common/filesystem/kqueue/watcher_impl.cc

+
+WatcherImpl::FileWatchPtr WatcherImpl::addWatch_(const std::string& path, uint32_t events,
+                                                 Watcher::OnChangedCb cb, bool pathMustExist) {
+  bool watchingDir = false;


nit: all variables should be snake_case (there are other examples).

mattklein123 · 2017-08-01T01:58:05Z

source/common/filesystem/kqueue/watcher_impl.cc

+    flags = NOTE_DELETE | NOTE_WRITE;
+  }
+
+  struct kevent event;


nit: struct not needed in C++ code.

The function kevent hides the struct, so I think it's necessary here.

mattklein123 · 2017-08-01T01:59:52Z

source/common/filesystem/kqueue/watcher_impl.h

+  void onKqueueEvent();
+  FileWatchPtr addWatch_(const std::string& path, uint32_t events, Watcher::OnChangedCb cb,
+                         bool pathMustExist);
+  void removeWatch_(FileWatchPtr& watch);


nit: removeWatch_ doesn't really need the trailing _.

mattklein123 · 2017-08-01T02:00:18Z

source/common/filesystem/kqueue/watcher_impl.cc

+        fmt::format("unable to add filesystem watch for file {}: {}", path, strerror(event.data)));
+  }
+
+  log_debug("added watch for file: '{}' fd: {}", path, watch_fd);


Please switch all of these to ENVOY_LOG macros.

htuch

What's the test coverage results like on the kqueue implementation?

zuercher · 2017-08-01T17:31:29Z

Coverage is about 85%. Everything but the error guards.

alyssawilk

Looks good overall, and very useful comments, thanks!

alyssawilk · 2017-08-01T18:05:05Z

source/common/filesystem/filesystem_impl.cc


 #include "common/common/assert.h"
 #include "common/common/thread.h"
+#include "common/filesystem/filesystem_impl.h"


Google include order has a special case for foo.cc including foo.h as the first line, so I think you can revert most of the include order changes.

I think the fix_format script will catch this for you, so it's worth running it locally and seeing what else it turns up!

I could swear the fix script did this for me. Weird.

alyssawilk · 2017-08-01T18:12:06Z

source/common/filesystem/kqueue/watcher_impl.cc

+
+  while (true) {
+    uint32_t events = 0;
+    int nevents = kevent(queue_, NULL, 0, &event, 1, &nullts);


NULL -> nullptr here?

alyssawilk · 2017-08-01T18:32:44Z

source/common/filesystem/kqueue/watcher_impl.cc

+
+void WatcherImpl::removeWatch(FileWatchPtr& watch) {
+  int fd = watch->fd_;
+  watches_.erase(fd);


I was surprised that one didn't have to unregister but kevent docs note "Events which are attached to file descriptors are automatically deleted on the last close of the descriptor."

Can you add a comment here that the deletion will close the fd and result in automatic unregistration?

alyssawilk · 2017-08-01T18:33:58Z

source/common/filesystem/kqueue/watcher_impl.cc

+    int watch_fd = reinterpret_cast<std::intptr_t>(event.udata);
+
+    FileWatchPtr file = watches_[watch_fd];
+    if (file == nullptr) {


I'm all for this paranoia check but when can this happen? Should it be an ASSERT that it does not?

alyssawilk · 2017-08-01T18:39:28Z

source/common/filesystem/kqueue/watcher_impl.cc

+          removeWatch(file);
+          file = new_file;
+
+          events |= Events::MovedTo;


Aren't the events in this case the events for the directory? Do we want to |= or overwrite entirely?

events is only ever 0 or Events::MovedTo (which is the only value defined in include/envoy/filesystem/filesystem.h). The inotify implementation does this as well. I assumed it was because someone expected to add more event types some day.

alyssawilk · 2017-08-01T18:43:15Z

source/common/filesystem/kqueue/watcher_impl.cc

+      file->callback_(events);
+    }
+
+    ENVOY_LOG(debug, "notification: fd: {} flags: {:x} file: {}", file->fd_, event.fflags,


Given the callback may trigger a bunch of logs on its own, you may want to bump this log line up before so you get the log about the event on the file before all the things triggered by that event.

alyssawilk

Looking good - just two more nits on my end

alyssawilk · 2017-08-01T19:50:17Z

source/common/filesystem/kqueue/watcher_impl.h

+  typedef std::shared_ptr<FileWatch> FileWatchPtr;
+
+  void onKqueueEvent();
+  FileWatchPtr addWatch_(const std::string& path, uint32_t events, Watcher::OnChangedCb cb,


I don't think this needs the trailing _ either

alyssawilk · 2017-08-01T19:58:55Z

test/common/filesystem/watcher_impl_test.cc

+  int rc = symlink(TestEnvironment::temporaryPath("envoy_test/watcher_target").c_str(),
+                   TestEnvironment::temporaryPath("envoy_test/watcher_new_link").c_str());
+  RELEASE_ASSERT(0 == rc);
+  UNREFERENCED_PARAMETER(rc);


Nice additions!

I wonder if the tests you're modeling predated gtest? I think you can replace these two lines with
ASSERT_EQ(0, rc); here and below and it'll be more consistent with tests elsewhere in the code base

alyssawilk · 2017-08-01T20:00:40Z

source/common/filesystem/kqueue/watcher_impl.cc

+    if (file->watching_dir_) {
+      if (event.fflags & NOTE_DELETE) {
+        // directory was deleted
+        removeWatch(file);


Does one of the tests cover this case?

Latest commit adds a test that hits this line.

Excellent, thanks!

alyssawilk · 2017-08-02T13:04:25Z

source/common/filesystem/inotify/watcher_impl.cc


 #include "common/common/assert.h"
 #include "common/common/utility.h"
+#include "common/filesystem/watcher_impl.h"


Sorry I missed this on the last pass but I think this include change still needs to be reverted

I'm guessing the new inotify prefix breaks the header format script (since this passed tests). I think it's probably not worth worrying about but we can potentially make a note to try to make the script deal with this somehow (by having it understand the strip stuff).

If it works for you, it works for me :-)

alyssawilk · 2017-08-02T13:04:51Z

source/common/filesystem/kqueue/watcher_impl.cc

+    if (file->watching_dir_) {
+      if (event.fflags & NOTE_DELETE) {
+        // directory was deleted
+        removeWatch(file);


Excellent, thanks!

mattklein123

Thank you @zuercher this looks great! Thanks for adding new tests also and fixing missing coverage. This works for me. Will hold off on merging in case @alyssawilk has any further comments/thoughts on the header order issue.

alyssawilk · 2017-08-02T14:27:19Z

LGTM on my front!

…oxy#1364) Automatic merge from submit-queue. Remove JWT headers consumed by Istio authn and mixer filters **What this PR does / why we need it**: This PR removes the JWT headers after they have been consumed by Istio authn and mixer filters. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes envoyproxy#1363 **Special notes for your reviewer**: **Release note**: ```release-note ```

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

filesystem: provide kqueue-based impl of Watcher

40e0db9

This was referenced Aug 1, 2017

hot restart: conditionally build hot restart #1365

Merged

osx support (fixes #128) #1348

Closed

mattklein123 reviewed Aug 1, 2017

View reviewed changes

htuch reviewed Aug 1, 2017

View reviewed changes

zuercher added 2 commits August 1, 2017 10:32

review comments

49169e8

fix tab/space

8eda98e

alyssawilk reviewed Aug 1, 2017

View reviewed changes

zuercher added 2 commits August 1, 2017 12:41

improve test coverage over kqueue watcher

6eb7c1b

review comments and format

1ba9ee1

alyssawilk reviewed Aug 1, 2017

View reviewed changes

address review comments

5885a89

alyssawilk reviewed Aug 2, 2017

View reviewed changes

mattklein123 approved these changes Aug 2, 2017

View reviewed changes

mattklein123 merged commit 374c3bf into envoyproxy:master Aug 2, 2017

zuercher deleted the osx-1-kqueue-watcher branch August 10, 2017 16:05

jpsim pushed a commit that referenced this pull request Nov 28, 2022

test: fix perf test with broken API usage (#1363)

2bcf617

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

test: fix perf test with broken API usage (#1363)

ee3b2f2

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

Conversation

zuercher commented Aug 1, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

zuercher commented Aug 1, 2017

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Aug 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants