Fix the concurrent map access crashing problem #162

haoel · 2022-07-05T03:39:16Z

Background

We have a map to collect all of the probers' probe results, all of the probers would set its result into this map concurrently in a different key(this is not a problem), however, we have another go routine that persistent this map periodically, this causes the "concurrent map iteration and map write" problem and it makes EaseProbe crash.

the stake trace of the crash as below:

fatal error: concurrent map iteration and map write

goroutine 50 [running]:
runtime.throw({0xfd17f1?, 0x0?})
        /usr/local/go/src/runtime/panic.go:992 +0x71 fp=0xc0172745f0 sp=0xc0172745c0 pc=0x438b31
runtime.mapiternext(0x0?)
        /usr/local/go/src/runtime/map.go:871 +0x4eb fp=0xc017274660 sp=0xc0172745f0 pc=0x41058b
runtime.mapiterinit(0xc0172747e8?, 0xc0265008d0?, 0xc017274710?)
        /usr/local/go/src/runtime/map.go:861 +0x228 fp=0xc017274680 sp=0xc017274660 pc=0x410048
reflect.mapiterinit(0xc0172746c8?, 0x8c24b3?, 0xc000303400?)
....
....
....
....
gopkg.in/yaml%2ev3.(*encoder).marshalDoc(0xc000303400, {0x0, 0x0}, {0xe7b4e0?, 0xc02a155740?, 0x0?})
        /home/ubuntu/go/pkg/mod/gopkg.in/yaml.v3@v3.0.0-20210107192922-496545a6307b/encode.go:105 +0x185 fp=0xc017275ab8 sp=0xc017275838 pc=0x8c2765
gopkg.in/yaml%2ev3.Marshal({0xe7b4e0?, 0xc02a155740})
        /home/ubuntu/go/pkg/mod/gopkg.in/yaml.v3@v3.0.0-20210107192922-496545a6307b/yaml.go:222 +0x370 fp=0xc017275de8 sp=0xc017275ab8 pc=0x8e2e70
gopkg.in/yaml%2ev3.Marshal({0xe7b4e0?, 0xc02a155740})
        /home/ubuntu/go/pkg/mod/gopkg.in/yaml.v3@v3.0.0-20210107192922-496545a6307b/yaml.go:222 +0x370 fp=0xc017275de8 sp=0xc017275ab8 pc=0x8e2e70
github.com/megaease/easeprobe/probe.SaveDataToFile({0xc0015858c0, 0x18})
        /home/ubuntu/hchen/easeprobe/probe/data.go:102 +0x85 fp=0xc017275e50 sp=0xc017275de8 pc=0x8e6ba5
main.saveData.func1()
        /home/ubuntu/hchen/easeprobe/cmd/easeprobe/report.go:34 +0x3d fp=0xc017275ee8 sp=0xc017275e50 pc=0xda975d
main.saveData(0xc000090780)
        /home/ubuntu/hchen/easeprobe/cmd/easeprobe/report.go:59 +0x16f fp=0xc017275fc8 sp=0xc017275ee8 pc=0xda958f
main.main.func3()
        /home/ubuntu/hchen/easeprobe/cmd/easeprobe/main.go:159 +0x26 fp=0xc017275fe0 sp=0xc017275fc8 pc=0xda7f66
runtime.goexit()
        /usr/local/go/src/runtime/asm_amd64.s:1571 +0x1 fp=0xc017275fe8 sp=0xc017275fe0 pc=0x46b6a1
created by main.main
        /home/ubuntu/hchen/easeprobe/cmd/easeprobe/main.go:159 +0x5fd

goroutine 1 [chan receive, 328 minutes]:
main.main()
        /home/ubuntu/hchen/easeprobe/cmd/easeprobe/main.go:205 +0x82b

the crash code line as blow:

// SaveDataToFile save the results to file
func SaveDataToFile(filename string) error {
        metaData.file = filename
        if strings.TrimSpace(filename) == "-" {
                return nil
        }

        dataBuf, err := yaml.Marshal(resultData)   //<========= Panic at this line
        if err != nil {
                return err
        }

        genMetaBuf()
        buf := append(metaBuf, dataBuf...)

        if err := ioutil.WriteFile(filename, []byte(buf), 0644); err != nil {
                return err
        }
        return nil
}

Investigation

The problem here is the prober's result (statistics data) would be consumed by the following modules:

Data Saving
SLA Report
Web Server

All of the above modules only read the statistics data, the probers would update the statistics data.

So, this is the read-write concurrent problem.

Solution

We transfer all of the probers statistics data into the Save Manager (via a channel), and all of the consumers(Saving, SLA Report, Web Server) retrieve the statistics data from Save Manager instead of the probers directly.

Each Prober retrieves the persistent data from Save Manager by a clone object.
Each Prober clones & reports its Result to Save Manager via a go channel.
Web Server and SLA Report would query the result data from Save Manager.
A Read-Write Lock added in Save Manager's Get/Set method.

                                                              ┌────────────┐
                                                   Query      │            │
  Result┌────────┐ retrieve                 ┌────────────────►│ Web Server │
┌───────┤ Prober │◄────────┐                │                 │            │
│       └────────┘         │                │                 └────────────┘
│                          │ Result. ┌──────┴──────┐
│ Result┌────────┐ retrieve│ Clone() │             │          ┌────────────┐
├───────┤ Prober │◄────────┼─────────┤ Save Manager│  Query   │            │
│       └────────┘         │         │   data.go   ├─────────►│ SLA Report │
│                          │    ┌────►             │          │            │
│ Result┌────────┐ retrieve│    │    └─────▲─┬─────┘          └────────────┘
├───────┤ Prober │◄────────┘    │          │ │
│       └────────┘              │          │ │  map[name]*Result
│                               │          │ │
│                               │     Load │ │ Save
│    Result.                    │      ┌───┴─▼────┐
│    Clone()  ┌──────────────┐  │      │          │
└────────────►│    Channel   ├──┘      │   File   │
              └──────────────┘         │          │
                                       └──────────┘

codecov-commenter · 2022-07-05T03:41:21Z

Codecov Report

Merging #162 (97fddcd) into main (00d5519) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #162      +/-   ##
==========================================
+ Coverage   92.17%   92.21%   +0.03%     
==========================================
  Files          46       45       -1     
  Lines        3323     3339      +16     
==========================================
+ Hits         3063     3079      +16     
  Misses        191      191              
  Partials       69       69

Impacted Files	Coverage Δ
probe/result.go	`91.66% <ø> (ø)`
probe/base/base.go	`100.00% <100.00%> (ø)`
probe/data.go	`65.18% <100.00%> (+1.90%)`	⬆️
report/filter.go	`94.05% <100.00%> (+0.12%)`	⬆️
report/sla.go	`94.26% <100.00%> (+0.16%)`	⬆️
daemon/daemon_unix.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00d5519...97fddcd. Read the comment docs.

probe/data.go

haoel · 2022-07-05T04:40:00Z

removed the unnecessary lock, and comment on the functions which needn't the lock.

probe/data.go

haoel · 2022-07-05T05:08:03Z

thanks, and I feel there still might be a problem because the result data is still updated by every prober...

haoel · 2022-07-05T07:25:50Z

New Solution

Each Prober retrieves the persistent data from Save Manager by a clone object.
Each Prober clones & reports its Result to Save Manager via a channel.
Save Manager no need for the lock. because there is no data sharing.

          Result
  report┌────────┐ retrieve
┌───────┤ Prober │◄────────┐
│       └────────┘         │
│         Result           │ Result. ┌─────────────┐
│ report┌────────┐ retrieve│ Clone() │             │
├───────┤ Prober │◄────────┼─────────┤ Save Manager│
│       └────────┘         │         │   data.go   │
│         Result           │    ┌────►             │
│ report┌────────┐ retrieve│    │    └─────▲─┬─────┘
├───────┤ Prober │◄────────┘    │          │ │
│       └────────┘              │          │ │  map[name]*Result
│                               │          │ │
│                               │          │ │
│    Result.                    │      ┌───┴─▼────┐
│    Clone()  ┌──────────────┐  │      │          │
└────────────►│  go  channel ├──┘      │   File   │
              └──────────────┘         │          │
                                       └──────────┘

xxx7xxxx · 2022-07-05T07:41:39Z

LGTM

proditis

LGTM

haoel · 2022-07-06T04:22:29Z

After carefully checking the code, I found the SLA Report also has the same problem, but it has a super low possibility.

I will do the refactoring for the SLA report after #158 is merged, otherwise, there will be many conflicts.

So, this PR is held at this moment.

… the problem

haoel · 2022-07-06T06:54:33Z

Note: push force because of syncing up with the main branch.

…esult

haoel · 2022-07-06T08:40:38Z

Updated Solution

Each Prober retrieves the persistent data from Save Manager by a clone object.
Each Prober clones & reports its Result to Save Manager via a channel.
Web Server and SLA Report would query the result data from Save Manager.
A Read-Write Lock added in Save Manager's Get/Set method.

                                                              ┌────────────┐
                                                   Query      │            │
  Result┌────────┐ retrieve                 ┌────────────────►│ Web Server │
┌───────┤ Prober │◄────────┐                │                 │            │
│       └────────┘         │                │                 └────────────┘
│                          │ Result. ┌──────┴──────┐
│ Result┌────────┐ retrieve│ Clone() │             │          ┌────────────┐
├───────┤ Prober │◄────────┼─────────┤ Save Manager│  Query   │            │
│       └────────┘         │         │   data.go   ├─────────►│ SLA Report │
│                          │    ┌────►             │          │            │
│ Result┌────────┐ retrieve│    │    └─────▲─┬─────┘          └────────────┘
├───────┤ Prober │◄────────┘    │          │ │
│       └────────┘              │          │ │  map[name]*Result
│                               │          │ │
│                               │     Load │ │ Save
│    Result.                    │      ┌───┴─▼────┐
│    Clone()  ┌──────────────┐  │      │          │
└────────────►│    Channel   ├──┘      │   File   │
              └──────────────┘         │          │
                                       └──────────┘

xxx7xxxx · 2022-07-06T09:16:19Z

LGTM, the deepcopy is a great way to fix race condition

localvar · 2022-07-06T10:01:46Z

/LGTM

haoel · 2022-07-07T05:22:53Z

I've tested this PR for 20 hours as below

20K TCP probe
5s probe interval
10 queries per second for SLA Report Page request.

Everything's fine so far. So, I am going to merge this PR.

samanhappy · 2022-07-26T01:57:59Z

Is this problem resolved? I got the same error yesterday, the stake trace of the crash as below:

fatal error: concurrent map iteration and map write

goroutine 29 [running]:
runtime.throw({0xfc89c8?, 0x0?})
	/opt/hostedtoolcache/go/1.18.3/x64/src/runtime/panic.go:992 +0x71 fp=0xc0003925f0 sp=0xc0003925c0 pc=0x438b31
runtime.mapiternext(0x0?)
	/opt/hostedtoolcache/go/1.18.3/x64/src/runtime/map.go:871 +0x4eb fp=0xc000392660 sp=0xc0003925f0 pc=0x41058b
runtime.mapiterinit(0xc0003927e8?, 0xc000377b60?, 0xc000392710?)
	/opt/hostedtoolcache/go/1.18.3/x64/src/runtime/map.go:861 +0x228 fp=0xc000392680 sp=0xc000392660 pc=0x410048
reflect.mapiterinit(0xc0003926c8?, 0x8c05b3?, 0xc000535000?)
	/opt/hostedtoolcache/go/1.18.3/x64/src/runtime/map.go:1373 +0x19 fp=0xc0003926a8 sp=0xc000392680 pc=0x4652d9
reflect.Value.MapKeys({0xe70440?, 0xc000307f88?, 0x0?})
	/opt/hostedtoolcache/go/1.18.3/x64/src/reflect/value.go:1644 +0xfd fp=0xc000392780 sp=0xc0003926a8 pc=0x4d2a1d
......

haoel · 2022-07-26T02:01:35Z

can I have a full stack?

samanhappy · 2022-07-26T02:05:46Z

full stack is here easeprobe.txt

haoel · 2022-07-26T02:32:45Z

@samanhappy thanks to report this issue.

I can see the following stack

github.com/megaease/easeprobe/probe.SaveDataToFile({0xfa8978, 0xe})
	/home/runner/work/easeprobe/easeprobe/probe/data.go:102 +0x85 fp=0xc000393e50 sp=0xc000393de8 pc=0x8e4ca5
main.saveData.func1()
	/home/runner/work/easeprobe/easeprobe/cmd/easeprobe/report.go:34 +0x3d fp=0xc000393ee8 sp=0xc000393e50 pc=0xda221d
main.saveData(0xc000078540)
	/home/runner/work/easeprobe/easeprobe/cmd/easeprobe/report.go:59 +0x16f fp=0xc000393fc8 sp=0xc000393ee8 pc=0xda204f
main.main.func3()
	/home/runner/work/easeprobe/easeprobe/cmd/easeprobe/main.go:159 +0x26 fp=0xc000393fe0 sp=0xc000393fc8 pc=0xda0cc6
runtime.goexit()
	/opt/hostedtoolcache/go/1.18.3/x64/src/runtime/asm_amd64.s:1571 +0x1 fp=0xc000393fe8 sp=0xc000393fe0 pc=0x46b6a1
created by main.main
	/home/runner/work/easeprobe/easeprobe/cmd/easeprobe/main.go:159 +0x5fd

but the current source code - probe/data.go:102 is not the probe.SaveDataToFile() function.

so, it looks like you use the old source code, could you please use the latest code?

samanhappy · 2022-07-26T02:39:40Z

got it, thanks, I was using docker, the image is not as new as github

haoel · 2022-07-26T02:41:57Z

got it, thanks, I was using docker, the image is not as new as github

Oh, this patch hasn't been released officially. You can build the docker image by yourself

make docker

haoel requested review from localvar, xxx7xxxx, proditis and zhao-kun July 5, 2022 03:39

localvar reviewed Jul 5, 2022

View reviewed changes

probe/data.go Outdated Show resolved Hide resolved

localvar reviewed Jul 5, 2022

View reviewed changes

probe/data.go Outdated Show resolved Hide resolved

xxx7xxxx reviewed Jul 5, 2022

View reviewed changes

probe/data.go Outdated Show resolved Hide resolved

probe/data.go Outdated Show resolved Hide resolved

xxx7xxxx reviewed Jul 5, 2022

View reviewed changes

probe/data.go Outdated Show resolved Hide resolved

localvar approved these changes Jul 5, 2022

View reviewed changes

xxx7xxxx approved these changes Jul 5, 2022

View reviewed changes

proditis approved these changes Jul 5, 2022

View reviewed changes

haoel added 8 commits July 6, 2022 14:47

fix the concurrent map access crash problem

6947c26

go fmt

45e7dc0

put the whole function CleanData() into the lock

ca715b0

remove the lock in LoadDataFromFile()

dae4b4f

remove the unnecessary lock, and comment why

556144d

go fmt

57afa64

correct the comment

8401a1b

New Solution: Using channel and clone result instead of lock to sovle…

81b2299

… the problem

haoel force-pushed the crash branch from b62e53e to 81b2299 Compare July 6, 2022 06:52

1) redirect the result access to data 2) add the rwlock for get/set r…

97fddcd

…esult

zhao-kun approved these changes Jul 7, 2022

View reviewed changes

zhao-kun merged commit bc50547 into megaease:main Jul 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the concurrent map access crashing problem #162

Fix the concurrent map access crashing problem #162

haoel commented Jul 5, 2022 •

edited

Loading

codecov-commenter commented Jul 5, 2022 •

edited

Loading

haoel commented Jul 5, 2022

haoel commented Jul 5, 2022

haoel commented Jul 5, 2022 •

edited

Loading

xxx7xxxx commented Jul 5, 2022

proditis left a comment

haoel commented Jul 6, 2022

haoel commented Jul 6, 2022

haoel commented Jul 6, 2022

xxx7xxxx commented Jul 6, 2022 •

edited

Loading

localvar commented Jul 6, 2022

haoel commented Jul 7, 2022

samanhappy commented Jul 26, 2022

haoel commented Jul 26, 2022

samanhappy commented Jul 26, 2022

haoel commented Jul 26, 2022

samanhappy commented Jul 26, 2022

haoel commented Jul 26, 2022

Fix the concurrent map access crashing problem #162

Fix the concurrent map access crashing problem #162

Conversation

haoel commented Jul 5, 2022 • edited Loading

Background

Investigation

Solution

codecov-commenter commented Jul 5, 2022 • edited Loading

Codecov Report

haoel commented Jul 5, 2022

haoel commented Jul 5, 2022

haoel commented Jul 5, 2022 • edited Loading

New Solution

xxx7xxxx commented Jul 5, 2022

proditis left a comment

Choose a reason for hiding this comment

haoel commented Jul 6, 2022

haoel commented Jul 6, 2022

haoel commented Jul 6, 2022

Updated Solution

xxx7xxxx commented Jul 6, 2022 • edited Loading

localvar commented Jul 6, 2022

haoel commented Jul 7, 2022

samanhappy commented Jul 26, 2022

haoel commented Jul 26, 2022

samanhappy commented Jul 26, 2022

haoel commented Jul 26, 2022

samanhappy commented Jul 26, 2022

haoel commented Jul 26, 2022

haoel commented Jul 5, 2022 •

edited

Loading

codecov-commenter commented Jul 5, 2022 •

edited

Loading

haoel commented Jul 5, 2022 •

edited

Loading

xxx7xxxx commented Jul 6, 2022 •

edited

Loading