Add filewatcher/v2 #654

fishy · 2024-05-22T23:07:48Z

Being the first lib implemented in baseplate.go and long predates generics, the API of filewatcher lacks type safety and can cause other annoyances.

Add filewatcher/v2 that provides:

Generics/type safety
Use io.Closer over Stop()
Use With- options over a Config struct
Use slog at error level over log wrapper

And change original filewatcher lib to a thin wrapper of v2.

This change intentionally does not change any of its users (experiments, secrets, etc.), as a proof that we did not break v0 API. The next change will switch them to use v2.

kylelemons · 2024-05-23T05:00:08Z

filewatcher/filewatcher.go

-
-	ctx    context.Context
-	cancel context.CancelFunc
+	result *v2.Result[any]


🔕 value rather than pointer?

v2.Result contains a field of type atomic.Value so it has copy-protection and can only be passed by pointer.

hmm, it recognizes value fields of sync.Mutex as long as you don't copy the containing struct... this doesn't seem like it should be different. Can you create the struct without copying?

non-pointer type copy-protected fields are appropriate when you just use their zero values in the constructors directly (e.g. locks, zero atomic values, etc.), for this, since it's a wrapper from v2, when constructing it you need to get the data from v2 and feed into this type, so a copy will always happen.

kylelemons · 2024-05-23T05:03:05Z

filewatcher/v2/filewatcher.go

+		d, mtime, files, err := openAndParse(path, parser, softLimit, hardLimit)
+		if err != nil {
+			slog.ErrorContext(r.ctx, "filewatcher: openAndParse returned error", "err", err)
+		} else {


🔕 return in the if block so you can outdent the else?

kylelemons · 2024-05-23T05:06:59Z

filewatcher/v2/filewatcher.go

+				if timer == nil {
+					timer = time.AfterFunc(fsEventsDelay, forceReload)
+				} else {
+					timer.Reset(fsEventsDelay)


reset is not safe to call if there is a chance that it might be expiring concurrently (which is basically always)

https://pkg.go.dev/time#Timer.Reset

if !t.Stop() { <-t.C } t.Reset(d)

the timer here was created via time.AfterFunc, if you look at its doccomment, it says:

The returned Timer's C field is not used and will be nil.

Whoops, yep. Got the timer and ticker confused.

Probably a good idea to add a comment about this, I could see someone reading this code later and being confused (since it relies not only on fully understanding what the code is doing, but also a specific line in an entirely separate piece of documentation).

I think the problem is that both time.NewTimer and time.AfterFunc creates a *time.Timer, but time.Timer.Reset has this caveat if it's created via time.NewTimer that you should do the additional check to avoid race condition, but that Reset itself is wrong (you just must always use it with some additional checks in certain conditions).

so overall I think the better approach is to be very careful to time.NewTimer instead of being careful to time.Timer.Reset. in most cases, you use time.NewTimer to do something after the timer fires, which is better done via time.AfterFunc which is much safer. or some other cases it might be able to be replaced by time.NewTicker (cases that the channel is important, for example to be used with select). I'm not sure if there are some cases you should use time.NewTimer over the other 2 (maybe there are some very niche cases).

added comments linking back to this thread.

kylelemons · 2024-05-23T05:09:46Z

filewatcher/v2/filewatcher.go

+}
+
+// The actual data held in Result.data.
+type atomicData[T any] struct {


the name atomicData to me implies a type that handles the atomicity itself, rather than the data stored inside an atomic.Value.

I would probably call it dataSnapshot or something to imply that it's the pairing of a data and its mtime

dataAndMtime it is :)

If you want to go that route, dataAtTime or dataWithTime or dataAt would read more smoothly I think.

ok dataAt it is then :)

kylelemons · 2024-05-23T05:11:45Z

filewatcher/v2/filewatcher.go

+// Config defines the config to be used in New function.
+//
+// Can be deserialized from YAML.
+type Config struct {


I wouldn't even have the Config in the v2 API.

You can have a filewatcherv1.OptionsForV2(Config) that returns a filewatcherv2.Option corresponding to whichever of its options were set to provide forward compatibility without having to carry forward the tightly coupled YAML

I copied v1 code into v2 directory and start modifying them and forgot to remove this one :)

kylelemons · 2024-05-23T05:18:17Z

filewatcher/v2/filewatcher.go

+		data, mtime, files, err = openAndParse(path, parser, opt.fileSizeLimit, hardLimit)
+		if errors.Is(err, fs.ErrNotExist) {
+			lastErr = err
+			time.Sleep(opt.initialReadInterval)


this is the business logic. for the initial load, if the file does not exit, we just sleep for initialReadInterval and keep retrying.

kylelemons · 2024-05-23T05:19:54Z

filewatcher/v2/filewatcher.go

+		data:  data,
+		mtime: mtime,
+	})
+	res.ctx, res.cancel = context.WithCancel(context.Background())


why not use the input ctx? or at least have a WithBackgroundContext option? you might not even need your close (unless people want to check its error) of you let the context contol its lifecycle

the ctx passed in controls how long people are willing to wait for the file to become available (see the doccomment of New), so it will not last for the whole lifecycle of the background checks (the whole lifecycle of the service/pod).

but on second thought since we have context.WithoutCancel now (we don't have that back in the days we implemented v0), it's probably appropriate to base from context.WithoutCancel(ctx) over context.Background(). made that change.

kylelemons · 2024-05-23T05:20:32Z

filewatcher/v2/filewatcher.go

+
+// NewMockFilewatcher returns a pointer to a new MockFileWatcher object
+// initialized with the given io.Reader and Parser.
+func NewMockFilewatcher[T any](r io.Reader, parser Parser[T]) (*MockFileWatcher[T], error) {


I would put this in filewatchertest rather than in the main file

kylelemons · 2024-05-23T05:21:10Z

filewatcher/v2/filewatcher.go

+//
+// This method is not threadsafe.
+func (fw *MockFileWatcher[T]) Update(r io.Reader) error {
+	data, err := fw.parser(r)


this is a fake, not a mock

This took me some time but now I'm fully sold on mock vs fakes.

kylelemons · 2024-05-23T05:21:35Z

filewatcher/v2/filewatcher.go

+// given DirParser.
+//
+// It provides Update function to update the data after it's been created.
+func NewMockDirWatcher[T any](dir fs.FS, parser DirParser[T]) (*MockDirWatcher[T], error) {


same two notes -- should be filewatchertest.NewFakeDirWatcher

konradreiche · 2024-05-23T06:11:16Z

filewatcher/v2/filewatcher.go

+			})
+			// remove all previously watched files
+			for _, path := range watcher.WatchList() {
+				watcher.Remove(path)


Do we care about logging the error here at all? Though I feel like it'll make it just unnecessarily noisy.

yea those will likely be very noisy.

konradreiche · 2024-05-23T06:33:57Z

filewatcher/v2/filewatcher.go

+//
+// This method is not threadsafe.
+func (fw *MockFileWatcher[T]) Update(r io.Reader) error {
+	data, err := fw.parser(r)


This took me some time but now I'm fully sold on mock vs fakes.

konradreiche · 2024-05-23T06:35:29Z

filewatcher/v2/filewatcher.go

+
+			switch ev.Op {
+			default:
+				// Ignore uninterested events.


I think we only ignore fsnotify.Chmod. Should we?

we don't have anything special to do with Chmod.

konradreiche · 2024-05-23T06:41:04Z

filewatcher/v2/filewatcher_test.go

+		m := make(map[string]string)
+		if err := fs.WalkDir(dir, ".", func(path string, de fs.DirEntry, err error) error {
+			if err != nil {
+				return nil


Suggested change

return nil

return err

Unless it's intentionally

it is intentional. in the lambda of WalkDir when you return non-nil error you stops the walk.

filewatcher/v2/filewatcher.go

mathyourlife-reddit · 2024-05-23T16:08:51Z

filewatcher/v2/filewatcher.go

+// Default to DefaultPollingInterval.
+// To disable polling completely, set it to a negative value.
+//
+// Without polling filewatcher relies solely on fs events from the parent


Suggested change

// Without polling filewatcher relies solely on fs events from the parent

// Without polling, filewatcher relies solely on fs events from the parent

mathyourlife-reddit · 2024-05-23T16:21:46Z

filewatcher/v2/filewatcher.go

+// Please note that this does not include errors returned by the first parser
+// call, which will be returned directly.
+func New[T any](ctx context.Context, path string, parser Parser[T], options ...Option) (*Result[T], error) {
+	opt := opts{


Could rewrite using the default options and then appending user provided options such as:

opt := opts{} // Default options optList := []Option{ WithFSEventsDelay(DefaultFSEventsDelay), WithPollingInterval(DefaultPollingInterval), WithInitialReadInterval(DefaultInitialReadInterval), WithFileSizeLimit(DefaultMaxFileSize), } // Default options get overridden by user provided options when processed in order. optList = append(optList, options...)

or using the WithOptions note above from @kylelemons

WithOptions( // Default options WithFSEventsDelay(DefaultFSEventsDelay), WithPollingInterval(DefaultPollingInterval), WithInitialReadInterval(DefaultInitialReadInterval), WithFileSizeLimit(DefaultMaxFileSize), // User options WithOptions(options...), )

konradreiche

Nice, also gave it a test as a replacement for v1 in one of our libraries. Everything is working as expected.

kylelemons · 2024-05-24T15:52:10Z

filewatcher/v2/filewatcher.go

+				if timer == nil {
+					timer = time.AfterFunc(fsEventsDelay, forceReload)
+				} else {
+					timer.Reset(fsEventsDelay)


Whoops, yep. Got the timer and ticker confused.

kylelemons · 2024-05-24T15:52:59Z

filewatcher/v2/filewatcher.go

+		defer ticker.Stop()
+		tickerChan = ticker.C
+	}
+	var timer *time.Timer


🔕

Suggested change

var timer *time.Timer

var timer *time.Timer // only populated with time.AfterFunc

kylelemons · 2024-05-24T15:54:03Z

filewatcher/v2/filewatcher.go

+}
+
+// The actual data held in Result.data.
+type atomicData[T any] struct {


If you want to go that route, dataAtTime or dataWithTime or dataAt would read more smoothly I think.

kylelemons · 2024-05-24T16:00:12Z

filewatcher/filewatcher.go

@@ -188,35 +189,35 @@ func New(ctx context.Context, cfg Config) (*Result, error) {
 	return &Result{result: result}, nil
 }

+// MockFileWatcher is an implementation of FileWatcher that does not actually read


this is also called a Mock when it's a Fake (or Stub, depending on how you think about "read only" types)

this is v1, renaming would be a breaking change.

kylelemons · 2024-05-24T16:01:15Z

filewatcher/filewatcher.go

@@ -225,29 +226,29 @@ func (fw *MockFileWatcher) Stop() {}
 // MockDirWatcher is an implementation of FileWatcher for testing with watching
 // directories.
 type MockDirWatcher struct {


also a fake/stub not mock

Being the first lib implemented in baseplate.go and long predates generics, the API of filewatcher lacks type safety and can cause other annoyances. Add filewatcher/v2 that provides: * Generics/type safety * Use io.Closer over Stop() * Use With- options over a Config struct * Use slog at error level over log wrapper And change original filewatcher lib to a thin wrapper of v2. Also add filewatcher/v2/fwtest and move the fakes there. This change intentionally does not change any of its users (experiments, secrets, etc.), as a proof that we did not break v0 API. The next change will switch them to use v2.

fishy requested a review from a team as a code owner May 22, 2024 23:07

fishy requested review from kylelemons, konradreiche and mathyourlife-reddit and removed request for a team May 22, 2024 23:07

fishy mentioned this pull request May 22, 2024

experiments: Switch to filewatcher/v2 #655

Merged

kylelemons reviewed May 23, 2024

View reviewed changes

konradreiche reviewed May 23, 2024

View reviewed changes

mathyourlife-reddit reviewed May 23, 2024

View reviewed changes

fishy force-pushed the filewatcher-v2 branch from f23d594 to 66a43a8 Compare May 23, 2024 17:43

fishy requested review from kylelemons, mathyourlife-reddit and konradreiche May 23, 2024 17:44

konradreiche approved these changes May 24, 2024

View reviewed changes

kylelemons approved these changes May 24, 2024

View reviewed changes

fishy force-pushed the filewatcher-v2 branch 3 times, most recently from cdae5ad to 68797d1 Compare May 24, 2024 16:41

fishy force-pushed the filewatcher-v2 branch from 68797d1 to 4eee114 Compare May 24, 2024 16:43

fishy merged commit 622dd70 into master May 24, 2024
2 checks passed

fishy deleted the filewatcher-v2 branch May 24, 2024 17:39

	// Without polling filewatcher relies solely on fs events from the parent
	// Without polling, filewatcher relies solely on fs events from the parent

	var timer *time.Timer
	var timer *time.Timer // only populated with time.AfterFunc

Add filewatcher/v2 #654

Add filewatcher/v2 #654

Conversation

fishy commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylelemons May 24, 2024 • 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

pacejackson May 24, 2024 • edited Loading

Choose a reason for hiding this comment

fishy May 24, 2024 • 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

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

konradreiche 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

kylelemons May 24, 2024 •

edited

Loading

pacejackson May 24, 2024 •

edited

Loading

fishy May 24, 2024 •

edited

Loading