Skip to content

Workhorse: remove the global Redis client

Mikhail Mazurskiy requested to merge ash2k/no-global-redis-client into master

What does this MR do and why?

While running tests locally for the first time, I got a unit test panic:

=== RUN   TestKeyChangesInstantReturn
=== RUN   TestKeyChangesInstantReturn/sees_change_with_key_existing_and_changed
--- FAIL: TestKeyChangesInstantReturn (0.00s)
    --- FAIL: TestKeyChangesInstantReturn/sees_change_with_key_existing_and_changed (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x8 pc=0x1054b0cc0]

goroutine 13 [running]:
testing.tRunner.func1.2({0x105761a80, 0x106029ed0})
	/opt/homebrew/opt/go/libexec/src/testing/testing.go:1545 +0x1c8
testing.tRunner.func1()
	/opt/homebrew/opt/go/libexec/src/testing/testing.go:1548 +0x360
panic({0x105761a80?, 0x106029ed0?})
	/opt/homebrew/opt/go/libexec/src/runtime/panic.go:914 +0x218
gitlab.com/gitlab-org/gitlab/workhorse/internal/redis.TestKeyChangesInstantReturn.func1(0x140000be701?)
	/Users/mike/src/gitlab/workhorse/internal/redis/keywatcher_test.go:118 +0x50
testing.tRunner(0x140005f3040, 0x140006079a0)
	/opt/homebrew/opt/go/libexec/src/testing/testing.go:1595 +0xe8
created by testing.(*T).Run in goroutine 12
	/opt/homebrew/opt/go/libexec/src/testing/testing.go:1648 +0x33c

So, I decided to fix it by checking errors properly in initRdb() and cleanly reporting them. Got this, which is much better than a panic:

=== RUN   TestKeyChangesInstantReturn
    keywatcher_test.go:23: 
        	Error Trace:	/Users/mike/src/gitlab/workhorse/internal/redis/keywatcher_test.go:23
        	            				/Users/mike/src/gitlab/workhorse/internal/redis/keywatcher_test.go:79
        	Error:      	Received unexpected error:
        	            	open ../../config.toml: no such file or directory
        	Test:       	TestKeyChangesInstantReturn
--- FAIL: TestKeyChangesInstantReturn (0.00s)

Then I decided to have a more holistic look at the code as that package-level field looks fishy. I think it's much nicer without it.

Screenshots or screen recordings

N/A

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Mikhail Mazurskiy

Merge request reports