Avoid race conditions in DNSSnooper's cached domains #2637

2opremio · 2017-06-22T13:02:16Z

rade

Do we really want such fine-grained locking? Rather than a single lock.

probe/endpoint/dns_snooper_linux_amd64.go

+	// Not worth using a RMutex since we don't expect a lot of contention
+	// and they are considerably larger (8 bytes vs 24 bytes), which would
+	// bloat the cache
+	sync.Mutex


2opremio · 2017-06-22T15:37:08Z

Do we really want such fine-grained locking? Rather than a single lock.

I started with a global lock, but I figured there would be some contention and locks are small (8 bytes).

As a nice side-effect, the code is more readable (it's obvious what the mutex guards).

rade

I would have done this differently, as per my comment, but if you want to squash & merge as is then that's fine.

probe/endpoint/dns_snooper_linux_amd64.go

+	// bloat the cache
+	mutex   *sync.Mutex
+	domains map[string]struct{}
+}


2opremio · 2017-06-23T09:20:26Z

Alright, I caved in (midsommar-good-mood :) ) and modified the code to use a global mutex instead.

rade

Small typo. Rest is LGTM.

probe/endpoint/dns_snooper_linux_amd64.go

+	stop       chan struct{}
+	pcapHandle *pcap.Handle
+	// gcache is goroutine-safe, but the
+	// values cached values aren't


Avoid race conditions in DNSSnooper's cached domains

4139494

rade reviewed Jun 22, 2017

View reviewed changes

Review feedback

22a31fc

rade reviewed Jun 22, 2017

View reviewed changes

probe/endpoint/dns_snooper_linux_amd64.go Outdated

// bloat the cache

mutex *sync.Mutex

domains map[string]struct{}

}

This comment was marked as abuse.

Sign in to view

Use a global lock instead

9778be7

rade suggested changes Jun 23, 2017

View reviewed changes

probe/endpoint/dns_snooper_linux_amd64.go Outdated

stop chan struct{}

pcapHandle *pcap.Handle

// gcache is goroutine-safe, but the

// values cached values aren't

This comment was marked as abuse.

Sign in to view

rade approved these changes Jun 23, 2017

View reviewed changes

Review feedback

4006040

2opremio merged commit 40bb28a into master Jun 23, 2017

2opremio deleted the 2636-DNSSnooper-race branch June 23, 2017 23:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid race conditions in DNSSnooper's cached domains #2637

Avoid race conditions in DNSSnooper's cached domains #2637

2opremio commented Jun 22, 2017

rade left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

2opremio commented Jun 22, 2017 •

edited

Loading

rade left a comment

This comment was marked as abuse.

2opremio commented Jun 23, 2017

rade left a comment

This comment was marked as abuse.

Avoid race conditions in DNSSnooper's cached domains #2637

Avoid race conditions in DNSSnooper's cached domains #2637

Conversation

2opremio commented Jun 22, 2017

rade left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

2opremio commented Jun 22, 2017 • edited Loading

rade left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

2opremio commented Jun 23, 2017

rade left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

2opremio commented Jun 22, 2017 •

edited

Loading