Segmentation violation after upgrade to v9 from v8

[niksteff]

Hi everyone,

we just upgraded from v8 to v9 in one of our backend services. We also migrated to the new repo address with this version upgrade. We are using go 1.19 btw.

After the upgrade we ran into an issue with a segmentation violation that was reproducible on our "integration" environment. We are using docker in kubernetes. The base image for the go binary is the alpine:latest image currently at version 3.17.2

Our code ran for month in production and we did not changes this part of the code in the last weeks at all. After we discovered and analyzed the problem, we reverted the upgrade from v9 to v8 and the problem is gone.

I don't want to blame the issue on the library yet as it could be a weird coincidence with the docker image, etc, but i am not sure either what to do from now.

Now here is the log of the seg violation and some code for reference. You will see we are using a redis.Pipeliner to Set(...) some values in redis.

Anybody as curious as i am what the issue could be here?

We are also using v9 in another service and did not yet experience the problem. But i am now quiet scared for deploying the upgrade to any service :D

overview facts:

• go 1.19
• redis v7
• go binary in docker container based on alpine 3.17.2 (amd64)
• upgrade redis lib from v8 to v9 and new repo address
• same code runs smooth in production in v8
• error only reproducible in the docker container not on local linux (amd64) or mac (m1) machines

this is the code in question

func persist(ctx context.Context, p redis.Pipeliner, in <-chan OurType) <-chan error {
	outErrs := make(chan error)

	go func() {
		defer close(outErrs)

		startTime := time.Now()
		startTimeUnix := startTime.Unix()

		builder := flatbuffers.NewBuilder(0)

		// for the redis operations
		var wg sync.WaitGroup

		// iterate the incoming data and store it in batches
		batchCount := 0
		for someObj := range in {
			batchCount++
			b := CreateStockAsBytes(builder, someObj, startTimeUnix) // convert object to flatbuffer representation

			// Add a pipelined command for each SET command
			p.Set(ctx, fmt.Sprintf("%s:%s", REDIS_KEY_SKU, someObj.No), string(b), 0)

			// when reaching the persist batch size store the items in the redis
			// db.
			if batchCount%REDIS_PERSIST_BATCH_SIZE == 0 {
				wg.Add(1)
				go func(wg *sync.WaitGroup) {
					defer wg.Done()

					_, err := p.Exec(ctx)
					if err != nil {
						outErrs <- fmt.Errorf("could not execute redis pipe: %w", err)
					}
				}(&wg)

				batchCount = 0
			}
		}

		// execute a possible non full batch
		if batchCount > 0 {
			wg.Add(1)
			go func(wg *sync.WaitGroup) {
				defer wg.Done()

				_, err := p.Exec(ctx)
				if err != nil {
					outErrs <- fmt.Errorf("could not execute redis pipe: %s", err.Error())
				}
			}(&wg)
		}

		wg.Wait() // wait for all redis operations to finish
	}()

please note i had to strip internal information from the log but the stacktrace is complete.

2023-02-16T07:42:08Z	INFO	service/persister.go:288	some logging text here i replaced for this post
unexpected fault address 0x27080
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x1 addr=0x27080 pc=0x469e69]

goroutine 163 [running]:
runtime.throw({0xbf6ee4?, 0x7f1b44d1f6e8?})
	/usr/local/go/src/runtime/panic.go:1047 +0x5d fp=0xc00005bb58 sp=0xc00005bb28 pc=0x43841d
runtime.sigpanic()
	/usr/local/go/src/runtime/signal_unix.go:842 +0x2c5 fp=0xc00005bba8 sp=0xc00005bb58 pc=0x44de65
runtime.memmove()
	/usr/local/go/src/runtime/memmove_amd64.s:339 +0x3c9 fp=0xc00005bbb0 sp=0xc00005bba8 pc=0x469e69
runtime.growslice(0xb875e0?, {0x0?, 0xc0001f0000?, 0x7f1b44f63598?}, 0x50?)
	/usr/local/go/src/runtime/slice.go:297 +0x534 fp=0xc00005bc18 sp=0xc00005bbb0 pc=0x44f494
github.com/redis/go-redis/v9.(*Pipeline).Process(0xc000b2c6f0, {0x40f227?, 0x58?}, {0xd27fa8?, 0xc005052ae0})
	/go/pkg/mod/github.com/redis/go-redis/v9@v9.0.2/pipeline.go:63 +0x58 fp=0xc00005bc70 sp=0xc00005bc18 pc=0x778298
github.com/redis/go-redis/v9.(*Pipeline).Process-fm({0xd24a70?, 0xc00011a000?}, {0xd27fa8?, 0xc005052ae0?})
	<autogenerated>:1 +0x49 fp=0xc00005bca8 sp=0xc00005bc70 pc=0x7f1c49
github.com/redis/go-redis/v9.cmdable.Set(0xc000b1de10, {0xd24a70?, 0xc00011a000}, {0xc019361390, 0xf}, {0xaef360?, 0xc0194cb660}, 0x0)
	/go/pkg/mod/github.com/redis/go-redis/v9@v9.0.2/commands.go:967 +0x41a fp=0xc00005bd28 sp=0xc00005bca8 pc=0x75223a
github.com/redis/go-redis/v9.(*Pipeline).Set(0xc00503be00?, {0xd24a70?, 0xc00011a000?}, {0xc019361390?, 0x2?}, {0xaef360?, 0xc0194cb660?}, 0x63edde50?)
	<autogenerated>:1 +0x4d fp=0xc00005bd78 sp=0xc00005bd28 pc=0x7fa78d
ourRepoPath/some-service/internal/service.persist.func1()
	/app/internal/service/persister.go:502 +0x3f0 fp=0xc00005bfe0 sp=0xc00005bd78 pc=0x81b3f0
runtime.goexit()
	/usr/local/go/src/runtime/asm_amd64.s:1594 +0x1 fp=0xc00005bfe8 sp=0xc00005bfe0 pc=0x468c01
created by ourRepoPath/some-service/internal/service.persist
	/app/internal/service/persister.go:484 +0xf9

[vmihailenco]

There is a breaking change that pipelines are not thread-safe any more: https://github.com/redis/go-redis/blob/master/CHANGELOG.md#breaking

[niksteff]

Thank you for the fast answer we will have a look if this could be the problem :)

[niksteff]

Yes that is the problem indeed. With your tip i could also find the commit where this was done etc.
What i cannot find is, why was this changed? Is there any specific problem that is mitigated by this? The changelog on the commit is a little thin - sorry i have to ask back here. Just states that it was done. I also found the change in the documentation on Pipeline now.

Because with this change right now, we definitly cannot upgrade as the risk of someone implementing it not threadsafe and not catching the problem in integration and unit tests is too high for us right now. I could write a linter rule but ... well.

I think we just have to stick to v8 for now - that solves our problem currently :)

Thank you again <3 and thank all your team for the nice work on the client as well.

[monkey92t]

It seems that there is a problem in our judgment, and many people want to use the pipeline thread-safely. Should we consider restoring it?

[vmihailenco]

It does not look too bad. Perhaps we could provide a thread-safe version of the Pipeliner? Something like NewLockedPipeline(Pipeliner) Pipeliner.

[monkey92t]

It does not look too bad. Perhaps we could provide a thread-safe version of the Pipeliner? Something like NewLockedPipeline(Pipeliner) Pipeliner.

After my test, each lock consumes about 10 ns/op (darwin amd64 core i5), we seem to be able to ignore it.

[Sheco]

Hello, I just hit this issue but I easily refactored without having to use the pipeline in different goroutines. It was a design fault on my side.