http2: share same hpack decoder for one tuple connect #744 #772

chilli13 · 2025-05-06T07:50:24Z

Copilot

Pull Request Overview

This PR refactors the handling of HTTP2 connections by sharing a single HPACK decoder per tuple connection and improves connection lifecycle management. Key changes include:

Modifying GetConn to return a pointer to ConnInfo instead of a tuple string.
Introducing a destroy connection channel and associated worker cleanup in the event processor.
Updating HTTP2 request and response implementations to reuse an HPACK decoder instance.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
user/module/probe_openssl.go	Updates GetConn return type and adds a call to WriteDestroyConn for valid socket IDs.
pkg/event_processor/processor.go	Adds destroyConn channel, destroyWorkers, and clearAllWorkers methods for managing worker lifecycles.
pkg/event_processor/iworker.go	Enhances the eventWorker interface with GetSock and Done; updates Run logic to handle ticker restarts and UUID parsing.
pkg/event_processor/http2_response.go	Reuses an HPACK decoder instance instead of creating a new one per call, with updated logging messages.
pkg/event_processor/http2_request.go	Applies similar HPACK decoder reuse as in responses and adds ClientPreface handling.

Copilot · 2025-05-06T12:18:19Z

pkg/event_processor/iworker.go

 			}
 			ew.tickerCount++
 		case e := <-ew.incoming:
+			if restartFlag {


When restartFlag is true, a new ticker is created without stopping the previous ticker, which could lead to resource leaks. Consider calling ticker.Stop() on the existing ticker before assigning a new one.

Suggested change

if restartFlag {

if restartFlag {

ew.ticker.Stop()

@Copilot ew.ticker.Stop() is already exec in function drainAndClose(ew) -> ew.Close() before here, so when restartFlag is true, it means ew.ticker is stopped status, no need to calling ticker.Stop()

cfc4n

感谢您的PR，不过，我觉得有很多功能需要改善一下。

另外，请务必修复 CI的错误。

pkg/event_processor/http2_request.go

cfc4n · 2025-05-06T12:34:21Z

pkg/event_processor/iworker.go

@@ -73,21 +79,43 @@ func NewEventWorker(uuid string, processor *EventProcessor) IWorker {
 	return eWorker
 }

+func getSock(uuid string) uint64 {


eCapture的事件类型可能不是socket， iWorker应该是一个抽象的对象。不应该把转化为socket的功能放到iwork里。

这个问题建议如何修改呢？ socket信息uuid 中再拆解？

iwork是一个不涉及具体包类型的数据处理类。传入的数据可能是base捕获的字符串，也可能是http、tcp的包。所以，socket这种具体业务的内容，我建议放到IParser这类代码中。

iwork是一个不涉及具体包类型的数据处理类。传入的数据可能是base捕获的字符串，也可能是http、tcp的包。所以，socket这种具体业务的内容，我建议放到IParser这类代码中。

当前修改方案是一个uuid（tuple+dateType确认）共享一个eWork，以解决hpack解码器共享问题。 destory发生时候通过通知destroyWorkers根据sock 匹配删除ework，将sock放到IParser如何实现eWork的add/delete管理？

cfc4n · 2025-05-06T12:35:34Z

pkg/event_processor/processor.go

@@ -33,6 +33,8 @@ type EventProcessor struct {
 	incoming chan event.IEventStruct
 	// send to output
 	outComing chan string
+	// destroyConn sock
+	destroyConn chan uint64


跟前面提到的iwork一样的问题，抽象上需要改进一下。不应该在processor里定义具体的socket的功能字段。

chilli13 · 2025-05-07T02:29:33Z

必修复 CI的错误。
testing 报错是环境问题吗？

    bpf_test.go:42: GetSystemConfig error:KernelConfig not found. with error: open /xxxxx/proc/config.gz: no such file or directory
Error:     bpf_test.go:46: TestBpfConfig with correct configuration (gzip compressed)

cfc4n · 2025-05-07T05:31:32Z

CI 错误：

https://github.com/gojue/ecapture/actions/runs/14855593137/job/41708068834?pr=772#step:10:32-125

=== RUN   TestEventProcessor_Serve
==================
WARNING: DATA RACE
Write at 0x00c000124268 by goroutine 12:
  github.com/gojue/ecapture/pkg/event_processor.(*EventProcessor).clearAllWorkers()
      /home/runner/work/ecapture/ecapture/pkg/event_processor/processor.go:156 +0x32d
  github.com/gojue/ecapture/pkg/event_processor.(*EventProcessor).Serve()
      /home/runner/work/ecapture/ecapture/pkg/event_processor/processor.go:84 +0x[31](https://github.com/gojue/ecapture/actions/runs/14855593137/job/41708068834?pr=772#step:10:32)3
  github.com/gojue/ecapture/pkg/event_processor.TestEventProcessor_Serve.func1()
      /home/runner/work/ecapture/ecapture/pkg/event_processor/processor_test.go:46 +0x37

Previous read at 0x00c000124268 by goroutine 11:
  github.com/gojue/ecapture/pkg/event_processor.(*EventProcessor).Close()
      /home/runner/work/ecapture/ecapture/pkg/event_processor/processor.go:190 +0x130
  github.com/gojue/ecapture/pkg/event_processor.TestEventProcessor_Serve()
      /home/runner/work/ecapture/ecapture/pkg/event_processor/processor_test.go:81 +0xd79
  testing.tRunner()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:1792 +0x225
  testing.(*T).Run.gowrap1()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:1851 +0x44

Goroutine 12 (running) created at:
  github.com/gojue/ecapture/pkg/event_processor.TestEventProcessor_Serve()
      /home/runner/work/ecapture/ecapture/pkg/event_processor/processor_test.go:44 +0x55c
  testing.tRunner()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:1792 +0x225
  testing.(*T).Run.gowrap1()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:1851 +0x44

Goroutine 11 (running) created at:
  testing.(*T).Run()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:1851 +0x8f2
  testing.runTests.func1()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:2279 +0x85
  testing.tRunner()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:1792 +0x225
  testing.runTests()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:2277 +0x96c
  testing.(*M).Run()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:2142 +0xeea
  main.main()
      _testmain.go:53 +0x164
==================
    processor_test.go:103: close error: EventProcessor.Close(): workerQueue is not empty:13
    testing.go:1490: race detected during execution of test
--- FAIL: TestEventProcessor_Serve (10.02s)
=== RUN   Test_Truncated_EventProcessor_Serve
==================
WARNING: DATA RACE
Write at 0x00c000090448 by goroutine 35:
  github.com/gojue/ecapture/pkg/event_processor.(*EventProcessor).clearAllWorkers()
      /home/runner/work/ecapture/ecapture/pkg/event_processor/processor.go:156 +0x[32](https://github.com/gojue/ecapture/actions/runs/14855593137/job/41708068834?pr=772#step:10:33)d
  github.com/gojue/ecapture/pkg/event_processor.(*EventProcessor).Serve()
      /home/runner/work/ecapture/ecapture/pkg/event_processor/processor.go:84 +0x313
  github.com/gojue/ecapture/pkg/event_processor.Test_Truncated_EventProcessor_Serve.func1()
      /home/runner/work/ecapture/ecapture/pkg/event_processor/processor_test.go:129 +0x37

Previous read at 0x00c000090448 by goroutine [34](https://github.com/gojue/ecapture/actions/runs/14855593137/job/41708068834?pr=772#step:10:35):
  github.com/gojue/ecapture/pkg/event_processor.(*EventProcessor).Close()
      /home/runner/work/ecapture/ecapture/pkg/event_processor/processor.go:190 +0x130
  github.com/gojue/ecapture/pkg/event_processor.Test_Truncated_EventProcessor_Serve()
      /home/runner/work/ecapture/ecapture/pkg/event_processor/processor_test.go:165 +0xcb9
  testing.tRunner()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:1792 +0x225
  testing.(*T).Run.gowrap1()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:1851 +0x44

Goroutine [35](https://github.com/gojue/ecapture/actions/runs/14855593137/job/41708068834?pr=772#step:10:36) (running) created at:
  github.com/gojue/ecapture/pkg/event_processor.Test_Truncated_EventProcessor_Serve()
      /home/runner/work/ecapture/ecapture/pkg/event_processor/processor_test.go:127 +0x55c
  testing.tRunner()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:1792 +0x225
  testing.(*T).Run.gowrap1()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:1851 +0x44

Goroutine 34 (running) created at:
  testing.(*T).Run()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:1851 +0x8f2
  testing.runTests.func1()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:2279 +0x85
  testing.tRunner()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:1792 +0x225
  testing.runTests()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:2277 +0x96c
  testing.(*M).Run()
      /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.1.linux-amd64/src/testing/testing.go:2142 +0xeea
  main.main()
      _testmain.go:53 +0x164
==================
    processor_test.go:177: Events truncated, size: 1000 bytes
    processor_test.go:182: close error: EventProcessor.Close(): workerQueue is not empty:2
    testing.go:1490: race detected during execution of test
--- FAIL: Test_Truncated_EventProcessor_Serve (10.01s)
FAIL
FAIL	github.com/gojue/ecapture/pkg/event_processor	20.0[39](https://github.com/gojue/ecapture/actions/runs/14855593137/job/41708068834?pr=772#step:10:40)s

Signed-off-by: chilli <chilli19890121@gmail.com>

zenyanle · 2025-05-26T11:39:55Z

为了让eventWorker和特定字段解耦，我认为可以新增一个类似isStatefulLongLived的bool类型字段来标识一个eventWorker需要维护一个长时间的有状态的事件解析器，在init阶段可以定义这类eventWorker的特定行为，这样既不用改变其他模块的eventWorker逻辑，也能对特定需求的eventWorker进行定制化处理，如果大家觉得这个思路有帮助，我可以尝试动手修改一下，并发起一个新的PR

zenyanle · 2025-05-26T13:05:34Z

@cfc4n 请问引入一个以上的eventWorker逻辑是否符合本项目的设计

cfc4n · 2025-05-26T14:27:17Z

@cfc4n 请问引入一个以上的eventWorker逻辑是否符合本项目的设计

引入isStatefulLongLived字段是合理的设计，不过，他的问题是处理特定的socket消息，希望你的设计可以优雅一点，抽象后可以支持这个需求。

Copilot

Pull Request Overview

This PR enhances connection tracking and HTTP/2 header decoding by:

Introducing a Sock identifier to link SSL events and manage connection lifecycles.
Adding a destroyConn channel and related logic to tear down workers when connections close.
Sharing a single HPACK decoder instance (hdec) per HTTP/2 connection instead of recreating one on each frame display.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
user/module/probe_openssl.go	Updated `GetConn` to return `*ConnInfo` and propagate `Sock`.
user/event/event_openssl.go	Added `Sock` to `SSLDataEvent` and extended `GetUUID` formatting.
pkg/event_processor/processor.go	Added `destroyConn` channel, `destroyWorkers`, and cleanup logic.
pkg/event_processor/iworker.go	Propagates `Sock` in workers and implements `GetSock`/`Done`.
pkg/event_processor/http2_request.go	Introduced per-connection `hdec` and reused it in `Display`.
pkg/event_processor/http2_response.go	Same per-connection `hdec` sharing for responses.

Copilot · 2025-05-27T12:55:50Z

pkg/event_processor/processor.go

@@ -105,6 +111,20 @@ func (ep *EventProcessor) dispatch(e event.IEventStruct) error {
 //	return this.incoming
 //}

+func (ep *EventProcessor) destroyWorkers(sock uint64) {


Accessing ep.workerQueue without acquiring ep.Lock() can lead to concurrent map read/write panics. Wrap the iteration over workerQueue in ep.Lock()/defer ep.Unlock() to ensure thread safety.

Copilot · 2025-05-27T12:55:50Z

pkg/event_processor/processor.go

@@ -132,6 +152,12 @@ func (ep *EventProcessor) delWorkerByUUID(worker IWorker) {
 	delete(ep.workerQueue, worker.GetUUID())
 }

+func (ep *EventProcessor) clearAllWorkers() {
+	ep.Lock()
+	defer ep.Unlock()


clearAllWorkers only resets the workerQueue map but does not signal existing workers to stop, potentially leaking goroutines. Consider calling Done() on each worker before clearing the map to properly shut them down.

Suggested change

defer ep.Unlock()

defer ep.Unlock()

for _, worker := range ep.workerQueue {

worker.Done()

}

Copilot · 2025-05-27T12:55:51Z

pkg/event_processor/http2_request.go

+	payloadLen := len(h2r.reader.String())
+	if payloadLen >= ClientPrefaceLen {
+		data := h2r.reader.String()[0:ClientPrefaceLen]


[nitpick] Calling reader.String() allocates a new string; use reader.Bytes() or reader.Len() on the underlying buffer to avoid unnecessary allocations.

Suggested change

payloadLen := len(h2r.reader.String())

if payloadLen >= ClientPrefaceLen {

data := h2r.reader.String()[0:ClientPrefaceLen]

payloadLen := h2r.reader.Len()

if payloadLen >= ClientPrefaceLen {

data := string(h2r.reader.Bytes()[0:ClientPrefaceLen])

Copilot · 2025-05-27T12:55:51Z

pkg/event_processor/iworker.go

+	uuidFileCount := 7
+	parts := strings.SplitN(uuid, "_", uuidFileCount)
+	if len(parts) != uuidFileCount {


[nitpick] The magic number 7 in SplitN(uuid, "_", 7) is brittle. Define a constant for the expected number of UUID segments to clarify its purpose and make future changes easier.

Suggested change

uuidFileCount := 7

parts := strings.SplitN(uuid, "_", uuidFileCount)

if len(parts) != uuidFileCount {

parts := strings.SplitN(uuid, "_", UUIDSegmentCount)

if len(parts) != UUIDSegmentCount {

chilli13 · 2025-06-11T06:05:42Z

I will create a new pr according to #785

dosubot bot added the enhancement New feature or request label May 6, 2025

chilli13 force-pushed the feat/http2-decode-hpack branch from 06b05e5 to c9c6805 Compare May 6, 2025 08:54

cfc4n requested a review from Copilot May 6, 2025 12:17

Copilot AI reviewed May 6, 2025

View reviewed changes

cfc4n requested changes May 6, 2025

View reviewed changes

http2: share same hpack decoder for one tuple connect gojue#744

35f82e7

Signed-off-by: chilli <chilli19890121@gmail.com>

chilli13 force-pushed the feat/http2-decode-hpack branch from c9c6805 to 35f82e7 Compare May 7, 2025 07:11

cfc4n requested a review from Copilot May 27, 2025 12:52

Copilot AI reviewed May 27, 2025

View reviewed changes

zenyanle mentioned this pull request May 28, 2025

feat: Implement dual lifecycle management for eventWorker #785

Merged

chilli13 closed this Jun 11, 2025

chilli13 deleted the feat/http2-decode-hpack branch July 22, 2025 02:46

http2: share same hpack decoder for one tuple connect #744 #772

http2: share same hpack decoder for one tuple connect #744 #772

Uh oh!

Conversation

chilli13 commented May 6, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 6, 2025

Choose a reason for hiding this comment

Uh oh!

chilli13 May 7, 2025

Choose a reason for hiding this comment

Uh oh!

cfc4n left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cfc4n May 6, 2025

Choose a reason for hiding this comment

Uh oh!

chilli13 May 7, 2025

Choose a reason for hiding this comment

Uh oh!

cfc4n May 8, 2025

Choose a reason for hiding this comment

Uh oh!

chilli13 May 8, 2025

Choose a reason for hiding this comment

Uh oh!

cfc4n May 6, 2025

Choose a reason for hiding this comment

Uh oh!

chilli13 commented May 7, 2025

Uh oh!

cfc4n commented May 7, 2025

Uh oh!

zenyanle commented May 26, 2025

Uh oh!

zenyanle commented May 26, 2025

Uh oh!

cfc4n commented May 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 27, 2025

Choose a reason for hiding this comment

Uh oh!

chilli13 commented Jun 11, 2025

Uh oh!

Uh oh!