Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

tracer: close channels on Stop() #49

Closed
wants to merge 2 commits into from

Conversation

alban
Copy link
Contributor

@alban alban commented Aug 8, 2017

PerfMap's PollStop() documentation says the channel should be closed
after calling PollStop():
https://github.com/iovisor/gobpf/blob/d0a3e1b/elf/perf.go#L241-L242

This is solving a leaking go routine issue in Scope:
weaveworks/scope#2735 (comment)


/cc @iaguis @2opremio @schu

@rade
Copy link
Member

rade commented Aug 8, 2017

btw PollStop is rather non-idiomatic - the typical way to do this kind of signalling is to close() a channel of type struct{}.

@alban
Copy link
Contributor Author

alban commented Aug 9, 2017

@rade ok, I filed iovisor/gobpf#69 for this.

@2opremio
Copy link
Contributor

2opremio commented Aug 9, 2017

I agree with @rade . I think it's worth fixing it now, but I am fine with fixing it later if you prefer.

@alban
Copy link
Contributor Author

alban commented Aug 9, 2017

idiomatic close in gobpf PR iovisor/gobpf#70

I'll update this PR shortly.

alban added 2 commits August 9, 2017 15:06
PerfMap's PollStop() documentation says the channel should be closed
after calling PollStop():
https://github.com/iovisor/gobpf/blob/d0a3e1b/elf/perf.go#L241-L242

This is solving a leaking go routine issue in Scope:
weaveworks/scope#2735 (comment)
Includes all gobpf changes up to iovisor/gobpf#70
@alban alban force-pushed the alban/close-channel branch from 3b3922b to b217e27 Compare August 9, 2017 13:09
alban added a commit to kinvolk-archives/scope that referenced this pull request Aug 9, 2017
This includes:

- weaveworks/tcptracer-bpf#49
  tracer: close channels on Stop()

- iovisor/gobpf#70
  perf: close go channels idiomatically
@alban
Copy link
Contributor Author

alban commented Aug 9, 2017

I updated the gobpf vendoring. I needed to change the select{} to avoid a panic ("panic: runtime error: index out of range"), see comment in the code.

It was working fine for a while but now I got another panic:

panic: send on closed channel

goroutine 75 [running]:
github.com/weaveworks/scope/vendor/github.com/weaveworks/tcptracer-bpf/vendor/github.com/iovisor/gobpf/elf.(*PerfMap).PollStart.func1(0xc420270370, 0xc422eff000, 0xc422eedfe0)
	/go/src/github.com/weaveworks/scope/vendor/github.com/weaveworks/tcptracer-bpf/vendor/github.com/iovisor/gobpf/elf/perf.go:229 +0x4f7
created by github.com/weaveworks/scope/vendor/github.com/weaveworks/tcptracer-bpf/vendor/github.com/iovisor/gobpf/elf.(*PerfMap).PollStart
	/go/src/github.com/weaveworks/scope/vendor/github.com/weaveworks/tcptracer-bpf/vendor/github.com/iovisor/gobpf/elf/perf.go:238 +0xf1

I am checking this...

@alban
Copy link
Contributor Author

alban commented Aug 9, 2017

channel closing principle:

One general principle of using Go channels is don't close a channel from the receiver side

This PR should be rethought..

@rade
Copy link
Member

rade commented Aug 9, 2017

@alban you beat me to quoting that :)

@alban
Copy link
Contributor Author

alban commented Aug 16, 2017

Waiting for iovisor/gobpf#71

@alban alban mentioned this pull request Aug 17, 2017
@alban
Copy link
Contributor Author

alban commented Aug 17, 2017

Closing in favor of #50

@alban alban closed this Aug 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants