fix: profileReporter internal event circulation #77

songzhibin97 · 2022-03-10T05:35:36Z

No description provided.

songzhibin97 · 2022-03-10T10:08:07Z

本质上面对的还是用户，如果发送到channel的消息不能够被成功的发送，合理吗~

另外，channel本来是解决速差的，如果选择发送是否可以在send的地方选择select适当丢弃？

Jun10ng · 2022-03-10T13:42:09Z

本质上面对的还是用户，如果发送到channel的消息不能够被成功的发送，合理吗~

另外，channel本来是解决速差的，如果选择发送是否可以在send的地方选择select适当丢弃？

你说服我了。
“如果发送到channel的消息不能够被成功的发送”这个确实是不合理的。

Jun10ng · 2022-03-10T13:49:02Z

holmes.go

 func (h *Holmes) ReportProfile(pType string, buf []byte, reason string, eventID string) {
+	if atomic.LoadInt64(&h.stopped) == 1 {
+		close(h.rptEventsCh)


这个地方是不是有bug会导致panic。
场景如下：
timeline1: ticke.c-> grDump() -> close(rptEventsCh) -> memDump -> close(rptEventsCh) ...
timeline2: stop holmes->

rptEventsCh 似乎会被close多次？因为stop标识位不会在Dump 过程中检查

close(h.rptEventsCh) 可以移到 h.Stop() 中？

嗯，应该放 stop 里，最好保留 stopReporter 这个函数名，语义更清晰

确实这里可能会有异常

嗯，应该放 stop 里，最好保留 stopReporter 这个函数名，语义更清晰

如果单独开一个stop方法，是否我们的start和stop的粒度就变了呢，我更倾向在发送端去close，当然我现在的做法可能有点问题，我需要修复一下

Jun10ng · 2022-03-10T13:51:57Z

能否增加一个单元测试：不stop holmes的同时只关闭reporter，然后再开启。
改造下 reporter_test.go 里的单元测试应该就可以，tks。

songzhibin97 · 2022-03-11T02:20:20Z

好的，我尝试一下

doujiang24 · 2022-03-11T07:43:15Z

holmes.go

+	h.rptEventsChLock.Lock()
+	h.rptEventsCh = make(chan rptEvent, 32)
+	h.rptEventsChLock.Unlock()


好多个锁...

我有个小建议，Start 里初始化这些 channel，stop 里关闭这些 channel
同时，start 和 stop 用 CompareAndSwap 保护一下重入

这样 race 应该足够小了，除非是 start 和 stop 的并发 race 了，这个我觉得这个很小的race 是可以接受的了

如果在stop上去close，有可能在发送端的时候发送消息的时候发送到close channel中

嗯，也对，close 得放在 sender 这里

我们搞成一个锁吧，fin 和 reporter 都放在 h.mutex 里，复用一下

holmes/holmes.go

Lines 284 to 289 in 17ac08a

h.goroutineCheckAndDump(gNum)

h.memCheckAndDump(mem)

h.cpuCheckAndDump(cpu)

h.threadCheckAndDump(tNum)

h.threadCheckAndShrink(tNum)

}

判断并关闭的操作放在每次dumpLoop函数的最后面如何，这样就不会有多次关闭channe的问题了，锁也可以删掉。
然后每个类型只需要判断stopped 然后 return。

想法不错，不过，gcheap 是另外一个循环，不在这个 loop 里呢

想法不错，不过，gcheap 是另外一个循环，不在这个 loop 里呢

gcheap 也做类似判断吧。
2处判断比 6处判断+锁竞争好一些。
@songzhibin97 你觉得呢

放外面的话感觉别人阅读代码是不是有点抽象

doujiang24 · 2022-03-15T07:15:46Z

holmes.go

+	ch := gc.h.getGcEventCh()
+	if ch == nil {
+		return
+	}
+


这段代码没必要了？

doujiang24 · 2022-03-15T07:16:06Z

holmes.go

+		if ch != nil {
+			close(gc.h.gcEventsCh)
+			gc.h.setGcEventCh(nil)
+		}


这段也可以删了？

doujiang24 · 2022-03-15T07:18:02Z

holmes.go

@@ -189,7 +229,6 @@ func (h *Holmes) Start() {
 // Stop the dump loop.
 func (h *Holmes) Stop() {
 	atomic.StoreInt64(&h.stopped, 1)


在这里 close channel，set channel = nil 就可以？在锁的保护下

doujiang24 · 2022-03-15T07:18:19Z

holmes.go

@@ -161,6 +200,7 @@ type gcHeapFinalizer struct {
 }

 func (h *Holmes) startGCCycleLoop() {
+	h.setGcEventCh(make(chan struct{}, 1))


这些 channel 初始化，可以统一放到 Start 函数里？

doujiang24 · 2022-03-15T07:19:18Z

holmes.go

-					continue
-				}
-				opts.reporter.Report(evt.PType, evt.Buf, evt.Reason, evt.EventID) // nolint: errcheck
+		ch := h.getRptEventsCh()


这里直接通过函数参数传参，更好

doujiang24 · 2022-03-15T07:21:32Z

holmes.go

+		return
+	}
+
+	if h.opts.rptOpts.allowDiscarding {


这个配置很鸡肋呢，要是不允许丢失，就阻塞主流程了，这个是更坏的影响

设计上来说，就不应该允许这种破坏主流程的行为

要想不丢失消息，有的是办法，消费者直接起协程来处理就可以

doujiang24 · 2022-03-15T07:23:07Z

holmes.go

+		return
+	}
+	// Waiting to be sent
+	ch <- rptEvent{


这种有问题的，如果发生了阻塞写，此时再 close 就 panic 了

doujiang24 · 2022-03-15T07:23:57Z

holmes_test.go

@@ -102,6 +102,9 @@ func createThread(n int, blockTime time.Duration) {
 }

 func TestWithShrinkThread(t *testing.T) {
+	h.Stop()


为啥要在这里加呢？单独起个用例更好一些

doujiang24 · 2022-03-16T01:24:08Z

go.mod

@@ -6,5 +6,5 @@ require (
 	github.com/gin-gonic/gin v1.7.7
 	github.com/shirou/gopsutil v3.20.11+incompatible
 	github.com/stretchr/testify v1.7.0
-	mosn.io/pkg v0.0.0-20211217101631-d914102d1baf
+	mosn.io/pkg v0.0.0-20220308091858-ea728aacbe63


请不要更新这个

doujiang24 · 2022-03-16T01:26:31Z

holmes.go

 		pType,
 		buf,
 		reason,
-		eventID}
+		eventID}:
+	default:


channel 满了的时候，最好加个 infof log

doujiang24 · 2022-03-16T01:27:20Z

holmes.go

 }

 // startReporter starts a background goroutine to consume event channel,
 // and finish it at after receive from cancel channel.
-func (h *Holmes) startReporter() {
+func (h *Holmes) startReporter(ch chan rptEvent) {
+	if ch == nil {


这个不能是 nil 了吧？

doujiang24 · 2022-03-16T01:30:24Z

holmes.go

@@ -191,17 +222,35 @@ func (h *Holmes) Start() {
 		h.Errorf("Holmes has started, please don't start it again.")
 		return
 	}
+	h.setGcEventCh(make(chan struct{}, 1))


这里的 channel 也可以当做参数往下传，可以简化很多逻辑

这两个 set channel，不用搞封装了，直接在这里显示 lock 就行了

doujiang24 · 2022-03-16T01:30:56Z

holmes.go

+// getGcEventCh get gcEventsCh
+func (h *Holmes) getGcEventCh() chan struct{} {
+	h.Lock()
+	defer h.Unlock()
+	return h.gcEventsCh
+}
+
+// setGcEventCh set gcEventsCh
+func (h *Holmes) setGcEventCh(ch chan struct{}) {
+	h.Lock()
+	defer h.Unlock()
+	h.gcEventsCh = ch
+}
+
+// getRptEventsCh get rptEventsCh
+func (h *Holmes) getRptEventsCh() chan rptEvent {
+	h.Lock()
+	defer h.Unlock()
+	return h.rptEventsCh
+}
+
+// setRptEventsCh set rptEventsCh
+func (h *Holmes) setRptEventsCh(ch chan rptEvent) {
+	h.Lock()
+	defer h.Unlock()
+	h.rptEventsCh = ch
+}
+


这些应该都没什么必要了，你可以捋一下的，传参可以简化很多

doujiang24 · 2022-03-16T07:02:19Z

提了一些改动在这个 PR：#85

songzhibin97 added 30 commits January 19, 2022 18:58

feat:cycle queue

9af816a

Merge remote-tracking branch 'upstream/master'

b970ab8

feat:split log

996fbde

fix:lint

b21a064

fix:lint

ed77cee

fix:lint

b2fc57a

fix:lint

43dee5c

fix:lint

1bf50cd

fix:lint

353a75a

Update log.go

8e35489

Update log.go

c7a605e

feat:option set human

9e42d73

fix:handle err

947b9d2

Update options.go

95564f7

Update options.go

071dd91

fix:明确语义

df33b7b

fix:切分日志出错时输出err

db2deb7

Update log.go

a7f889b

fix:default specify error

ddbfdfe

Merge remote-tracking branch 'upstream/master'

e3d1726

fix: trimResult optimize && supplementar comments

4c4f18d

Merge remote-tracking branch 'upstream/master'

1d8ea1a

fix:trimResult optimize

7165a16

fix:abstract constant

f9ff8e2

fix:lint

3a7321f

Update consts.go

a6904a3

fix:example

cb92f30

fix:初始化切分日志判断&trimResult修正

4126c5c

Merge remote-tracking branch 'upstream/master'

027eb2c

Merge remote-tracking branch 'upstream/master'

854b263

Jun10ng reviewed Mar 10, 2022

View reviewed changes

songzhibin97 mentioned this pull request Mar 11, 2022

bug: GCHeap check can not start again after stop #79

Closed

fix:suggestion

72285fa

mosn-community-bot bot added size/L and removed size/M labels Mar 11, 2022

songzhibin97 added 2 commits March 11, 2022 12:16

Update reporter_test.go

82b5d23

Update holmes_test.go

a6d50a4

doujiang24 reviewed Mar 11, 2022

View reviewed changes

songzhibin97 added 2 commits March 14, 2022 10:54

feat:common lock

debb0c8

fix:remove lock

41d2216

doujiang24 reviewed Mar 15, 2022

View reviewed changes

songzhibin97 and others added 6 commits March 15, 2022 16:36

fix:suggestion

2acb701

Merge branch 'master' into master

2cbd6d5

fix:rebase

5b2353a

Update holmes.go

ba9e40c

Update holmes.go

2d8b9ee

Update reporter_test.go

c75e74c

doujiang24 reviewed Mar 16, 2022

View reviewed changes

fix:suggestion

697023b

doujiang24 approved these changes Mar 16, 2022

View reviewed changes

doujiang24 merged commit edf4f5d into mosn:master Mar 16, 2022

doujiang24 added a commit that referenced this pull request Mar 16, 2022

fix some issues in #77. eg. add recover in ReportProfile to catch panic.

5694306

doujiang24 added a commit that referenced this pull request Mar 16, 2022

fix some issues in #77. eg. add recover in ReportProfile to catch panic.

07c380b

doujiang24 added a commit that referenced this pull request Mar 16, 2022

fix some issues in #77. eg. add recover to catch panic.

249b589

doujiang24 added a commit that referenced this pull request Mar 16, 2022

fix some issues in #77. eg. add recover to catch panic. (#85)

a389e31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: profileReporter internal event circulation #77

fix: profileReporter internal event circulation #77

songzhibin97 commented Mar 10, 2022

songzhibin97 commented Mar 10, 2022

Jun10ng commented Mar 10, 2022

Jun10ng Mar 10, 2022

Jun10ng Mar 10, 2022

doujiang24 Mar 11, 2022

songzhibin97 Mar 11, 2022

songzhibin97 Mar 11, 2022

Jun10ng commented Mar 10, 2022

songzhibin97 commented Mar 11, 2022

doujiang24 Mar 11, 2022

songzhibin97 Mar 11, 2022

doujiang24 Mar 14, 2022

Jun10ng Mar 14, 2022

doujiang24 Mar 14, 2022

Jun10ng Mar 14, 2022 •

edited

Loading

songzhibin97 Mar 14, 2022

doujiang24 Mar 15, 2022

doujiang24 Mar 15, 2022

doujiang24 Mar 15, 2022

doujiang24 Mar 15, 2022

doujiang24 Mar 15, 2022

doujiang24 Mar 15, 2022

doujiang24 Mar 15, 2022

doujiang24 Mar 15, 2022

doujiang24 Mar 15, 2022

doujiang24 Mar 16, 2022

doujiang24 Mar 16, 2022

doujiang24 Mar 16, 2022

doujiang24 Mar 16, 2022

doujiang24 Mar 16, 2022

doujiang24 commented Mar 16, 2022

	h.goroutineCheckAndDump(gNum)
	h.memCheckAndDump(mem)
	h.cpuCheckAndDump(cpu)
	h.threadCheckAndDump(tNum)
	h.threadCheckAndShrink(tNum)
	}

fix: profileReporter internal event circulation #77

fix: profileReporter internal event circulation #77

Conversation

songzhibin97 commented Mar 10, 2022

songzhibin97 commented Mar 10, 2022

Jun10ng commented Mar 10, 2022

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

Jun10ng commented Mar 10, 2022

songzhibin97 commented Mar 11, 2022

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

Jun10ng Mar 14, 2022 • 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

doujiang24 commented Mar 16, 2022

Jun10ng Mar 14, 2022 •

edited

Loading