Remove reflect from chanotify and fix a deadlock case #77

rakyll · 2016-01-20T23:00:01Z

With the change, the read rate from (*Notifier).Chan() is 10-20 times faster and more consistent.

Consider the following program:

  s := chanotify.New()
  for i := 0; i < n; i++ {
    ch := make(chan struct{}, 1)
    s.Add(ch, fmt.Sprintf("%d", i))
    go func(ch chan struct{}) {
      time.Sleep(time.Second) // because your original code has a deadlock case.
      ch <- struct{}{}
    }(ch)
  }

  avgs := make([]int64, n)
  go func() {
    for i := 0; i < n; i++ {
      start := time.Now()
      <-s.Chan()
      avgs[i] = time.Now().Sub(start).Nanoseconds()
    }
  }()

  time.Sleep(10 * time.Second)
  s.Close()

  fmt.Println(avgs)

The output without the change; ignore the first value:

[1000469322 739 100492 200 75412 77733 316 88873 695 137905 244 72197 196 84444 175 80858 169 125514 165 73509 885 739963 248 72569 169 90094 159 110571 68954 143 145616 148 83563 149 86154 132 82722 154 79740 170 86688 143 97033 158 87126 187 69839 125 100043 148 72633 133 80690 149 41841 113509 346 134876 247 80720 153473 414352 293 103906 276 140524 233 88041 236 123732 242 89870 238 105342 213 110773 319 121004 228 89237 793 94458 235 604864 400129 412 82639 598 72319 178 64423 157 35779 44536 235 55411 129 46051 29032 231]

The output with the change; ignore the first value:

[999893266 3189 2514 2257 2119 2252 2780 3402 2689 2916 2218 2385 4459 495 309 1289 578 4432 590 421 387 335 1156 272 1566 4933 1271 537 391 792 373 329 411 527 1764 782 322 1044 718 533 405 1183 337 230 1827 848 575 692 321 3514 504 491 772 1952 575 2931 1754 1279 781 403 1137 451 953 914 369 387 289 1796 473 1237 798 816 1215 690 495 389 403 1235 373 364 515 509 321 349 319 41810 27359 2582 2055 2177 2486 2181 1903 2207 2207 2005 1746 1802 1864 2169]

The change also fixes the deadlock case pointed in the program above.

LK4D4 · 2016-01-20T23:09:26Z

Makes sense

LK4D4 · 2016-01-20T23:10:36Z

chanotify/chanotify.go

 	s.m.Unlock()
-	s.ctrl <- struct{}{}
+	close(done)


Why do you need to close done here? As I see there won't be any reads from it at this point.

With the change, the read rate from (*Notifier).Chan() is 10-20 times faster and more consistent. Consider the following program: s := chanotify.New() for i := 0; i < n; i++ { ch := make(chan struct{}, 1) s.Add(ch, fmt.Sprintf("%d", i)) go func(ch chan struct{}) { time.Sleep(time.Second) // because your original code has a deadlock case. ch <- struct{}{} }(ch) } avgs := make([]int64, n) go func() { for i := 0; i < n; i++ { start := time.Now() <-s.Chan() avgs[i] = time.Now().Sub(start).Nanoseconds() } }() time.Sleep(10 * time.Second) s.Close() fmt.Println(avgs) The output without the change; ignore the first value: [1000469322 739 100492 200 75412 77733 316 88873 695 137905 244 72197 196 84444 175 80858 169 125514 165 73509 885 739963 248 72569 169 90094 159 110571 68954 143 145616 148 83563 149 86154 132 82722 154 79740 170 86688 143 97033 158 87126 187 69839 125 100043 148 72633 133 80690 149 41841 113509 346 134876 247 80720 153473 414352 293 103906 276 140524 233 88041 236 123732 242 89870 238 105342 213 110773 319 121004 228 89237 793 94458 235 604864 400129 412 82639 598 72319 178 64423 157 35779 44536 235 55411 129 46051 29032 231] The output with the change; ignore the first value: [999893266 3189 2514 2257 2119 2252 2780 3402 2689 2916 2218 2385 4459 495 309 1289 578 4432 590 421 387 335 1156 272 1566 4933 1271 537 391 792 373 329 411 527 1764 782 322 1044 718 533 405 1183 337 230 1827 848 575 692 321 3514 504 491 772 1952 575 2931 1754 1279 781 403 1137 451 953 914 369 387 289 1796 473 1237 798 816 1215 690 495 389 403 1235 373 364 515 509 321 349 319 41810 27359 2582 2055 2177 2486 2181 1903 2207 2207 2005 1746 1802 1864 2169] The change also fixes the deadlock case pointed in the program above. Signed-off-by: Burcu Dogan <jbd@google.com>

rakyll · 2016-01-20T23:35:15Z

PTAL

LK4D4 · 2016-01-20T23:58:45Z

LGTM

rakyll · 2016-01-21T00:12:43Z

The build is failing for some other reason, could you fix it?

LK4D4 · 2016-01-21T00:14:36Z

@rakyll not until we have vendored deps :/

crosbymichael · 2016-01-21T19:53:05Z

LGTM

Remove reflect from chanotify and fix a deadlock case

Relevant changes: - containerd/containerd#51 Fix empty device type - containerd/containerd#52 Remove call to unitName - Calling unitName incorrectly appends -slice onto the end of the slice cgroup we are looking for - addresses containerd/containerd#47 cgroups: cgroup deleted - containerd/containerd#53 systemd-239+ no longer allows delegate slice - containerd/containerd#54 Bugfix: can't write to cpuset cgroup - containerd/containerd#63 Makes Load function more lenient on subsystems' checking - addresses containerd/containerd#58 Very strict checking of subsystems' existence while loading cgroup - containerd/containerd#67 Add functionality for retrieving all tasks of a cgroup - containerd/containerd#68 Fix net_prio typo - containerd/containerd#69 Blkio weight/leafWeight pointer value - containerd/containerd#77 Check for non-active/supported cgroups - addresses containerd/containerd#76 unable to find * in controller set: unknown - addresses docker/for-linux#545 Raspbian: Error response from daemon: unable to find "net_prio" in controller set: unknown - addresses docker/for-linux#552 Error response from daemon: unable to find "cpuacct" in controller set: unknown - addresses docker/for-linux#545 Raspbian: Error response from daemon: unable to find "net_prio" in controller set: unknown Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Relevant changes: - containerd/containerd#51 Fix empty device type - containerd/containerd#52 Remove call to unitName - Calling unitName incorrectly appends -slice onto the end of the slice cgroup we are looking for - addresses containerd/containerd#47 cgroups: cgroup deleted - containerd/containerd#53 systemd-239+ no longer allows delegate slice - containerd/containerd#54 Bugfix: can't write to cpuset cgroup - containerd/containerd#63 Makes Load function more lenient on subsystems' checking - addresses containerd/containerd#58 Very strict checking of subsystems' existence while loading cgroup - containerd/containerd#67 Add functionality for retrieving all tasks of a cgroup - containerd/containerd#68 Fix net_prio typo - containerd/containerd#69 Blkio weight/leafWeight pointer value - containerd/containerd#77 Check for non-active/supported cgroups - addresses containerd/containerd#76 unable to find * in controller set: unknown - addresses docker/for-linux#545 Raspbian: Error response from daemon: unable to find "net_prio" in controller set: unknown - addresses docker/for-linux#552 Error response from daemon: unable to find "cpuacct" in controller set: unknown - addresses docker/for-linux#545 Raspbian: Error response from daemon: unable to find "net_prio" in controller set: unknown Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 386b06eacd689bb98fb4117087614466c6e130f3 Component: engine

Relevant changes: - containerd/containerd#51 Fix empty device type - containerd/containerd#52 Remove call to unitName - Calling unitName incorrectly appends -slice onto the end of the slice cgroup we are looking for - addresses containerd/containerd#47 cgroups: cgroup deleted - containerd/containerd#53 systemd-239+ no longer allows delegate slice - containerd/containerd#54 Bugfix: can't write to cpuset cgroup - containerd/containerd#63 Makes Load function more lenient on subsystems' checking - addresses containerd/containerd#58 Very strict checking of subsystems' existence while loading cgroup - containerd/containerd#67 Add functionality for retrieving all tasks of a cgroup - containerd/containerd#68 Fix net_prio typo - containerd/containerd#69 Blkio weight/leafWeight pointer value - containerd/containerd#77 Check for non-active/supported cgroups - addresses containerd/containerd#76 unable to find * in controller set: unknown - addresses docker/for-linux#545 Raspbian: Error response from daemon: unable to find "net_prio" in controller set: unknown - addresses docker/for-linux#552 Error response from daemon: unable to find "cpuacct" in controller set: unknown - addresses docker/for-linux#545 Raspbian: Error response from daemon: unable to find "net_prio" in controller set: unknown Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 386b06e) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Relevant changes: - containerd/containerd#51 Fix empty device type - containerd/containerd#52 Remove call to unitName - Calling unitName incorrectly appends -slice onto the end of the slice cgroup we are looking for - addresses containerd/containerd#47 cgroups: cgroup deleted - containerd/containerd#53 systemd-239+ no longer allows delegate slice - containerd/containerd#54 Bugfix: can't write to cpuset cgroup - containerd/containerd#63 Makes Load function more lenient on subsystems' checking - addresses containerd/containerd#58 Very strict checking of subsystems' existence while loading cgroup - containerd/containerd#67 Add functionality for retrieving all tasks of a cgroup - containerd/containerd#68 Fix net_prio typo - containerd/containerd#69 Blkio weight/leafWeight pointer value - containerd/containerd#77 Check for non-active/supported cgroups - addresses containerd/containerd#76 unable to find * in controller set: unknown - addresses docker/for-linux#545 Raspbian: Error response from daemon: unable to find "net_prio" in controller set: unknown - addresses docker/for-linux#552 Error response from daemon: unable to find "cpuacct" in controller set: unknown - addresses docker/for-linux#545 Raspbian: Error response from daemon: unable to find "net_prio" in controller set: unknown Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 386b06eacd689bb98fb4117087614466c6e130f3) Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: a36f6bfc36d2425c693097102b91152fa8f80012 Component: engine

Relevant changes: - containerd/containerd#51 Fix empty device type - containerd/containerd#52 Remove call to unitName - Calling unitName incorrectly appends -slice onto the end of the slice cgroup we are looking for - addresses containerd/containerd#47 cgroups: cgroup deleted - containerd/containerd#53 systemd-239+ no longer allows delegate slice - containerd/containerd#54 Bugfix: can't write to cpuset cgroup - containerd/containerd#63 Makes Load function more lenient on subsystems' checking - addresses containerd/containerd#58 Very strict checking of subsystems' existence while loading cgroup - containerd/containerd#67 Add functionality for retrieving all tasks of a cgroup - containerd/containerd#68 Fix net_prio typo - containerd/containerd#69 Blkio weight/leafWeight pointer value - containerd/containerd#77 Check for non-active/supported cgroups - addresses containerd/containerd#76 unable to find * in controller set: unknown - addresses docker/for-linux#545 Raspbian: Error response from daemon: unable to find "net_prio" in controller set: unknown - addresses docker/for-linux#552 Error response from daemon: unable to find "cpuacct" in controller set: unknown - addresses docker/for-linux#545 Raspbian: Error response from daemon: unable to find "net_prio" in controller set: unknown Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Check for non-active/supported cgroups

Bump to package version 1.2.2-2

GordonTheTurtle added status/0-triage labels Jan 20, 2016

rakyll force-pushed the master branch from c4dd79d to 1116a10 Compare January 20, 2016 23:07

GordonTheTurtle removed the dco/no label Jan 20, 2016

LK4D4 reviewed Jan 20, 2016
View reviewed changes

rakyll force-pushed the master branch from 1116a10 to 0cd5c21 Compare January 20, 2016 23:34

rakyll mentioned this pull request Jan 21, 2016

make chanotify to work with interface{} keys #79

Closed

crosbymichael added a commit that referenced this pull request Jan 21, 2016

Merge pull request #77 from rakyll/master

e5ea745

Remove reflect from chanotify and fix a deadlock case

crosbymichael merged commit e5ea745 into containerd:master Jan 21, 2016

jepio pushed a commit to jepio/containerd that referenced this pull request Dec 3, 2021

Merge pull request containerd#77 from crosbymichael/non-active

dbea6f2

Check for non-active/supported cgroups

jseba pushed a commit to jseba/containerd that referenced this pull request Aug 25, 2023

Merge pull request containerd#77 from seemethere/bump

26d2499

Bump to package version 1.2.2-2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove reflect from chanotify and fix a deadlock case #77

Remove reflect from chanotify and fix a deadlock case #77

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Remove reflect from chanotify and fix a deadlock case #77

Remove reflect from chanotify and fix a deadlock case #77

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!