Fix Subscribe panic and add a test which would have caught it #74

nshalman · 2022-01-06T20:16:45Z

Description

This PR creates a new test for the Subscribe endpoint as well as fixing a current bug which causes panics when that RPC is used.

Why is this needed

Fixes: #69

How Has This Been Tested?

~/Workspace/hegel % go test -run TestSubscribe -count 2000 ./grpc-server/...
ok  	github.com/tinkerbell/hegel/grpc-server	15.102s

~/Workspace/hegel % git revert --no-edit 88728c4
Auto-merging grpc-server/grpc_server.go
[nshalman/subscribe-fix-and-test 1d53e08] Revert "subLock needs to be initialized"
 Date: Fri Jan 7 13:14:24 2022 -0500
 1 file changed, 1 deletion(-)

~/Workspace/hegel % go test -run TestSubscribe ./grpc-server/...
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x107525e]

goroutine 104 [running]:
sync.(*RWMutex).Lock(0xc00031d080)
<snip>

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

codecov · 2022-01-06T20:40:46Z

Codecov Report

Merging #74 (4b367ad) into main (bb62536) will increase coverage by 16.10%.
The diff coverage is 58.33%.

@@             Coverage Diff             @@
##             main      #74       +/-   ##
===========================================
+ Coverage   33.89%   50.00%   +16.10%     
===========================================
  Files           3        3               
  Lines         413      422        +9     
===========================================
+ Hits          140      211       +71     
+ Misses        250      177       -73     
- Partials       23       34       +11

Impacted Files	Coverage Δ
grpc-server/grpc_server.go	`54.33% <58.33%> (+40.31%)`	⬆️

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 bb62536...4b367ad. Read the comment docs.

nshalman · 2022-01-07T16:41:19Z

Thanks again to @mmlb for most of this test harness, and for the pointers for the further cleanup of the tests in https://github.com/tinkerbell/hegel/compare/669989512f74834d3a41cbcc271cf03cb152c630..f0ae94cd34199f0d1d1ce3c90e64dfb8ad9111a5

mmlb · 2022-01-07T16:58:55Z

Thanks again to @mmlb for most of this test harness, and for the pointers for the further cleanup of the tests in 669989512f74834d3a41cbcc271cf03cb152c630..f0ae94cd34199f0d1d1ce3c90e64dfb8ad9111a5 (compare)

np. now that I look again the WaitGroup is probably unnecessary and I forgot to drop it

nshalman · 2022-01-07T17:25:23Z

now that I look again the WaitGroup is probably unnecessary and I forgot to drop it

Good catch. I will clean that up.

Edit: Cleaned up in https://github.com/tinkerbell/hegel/compare/f0ae94cd34199f0d1d1ce3c90e64dfb8ad9111a5..b149044946bc8f3de8db4607f3851b37e9e3d75a

tobert

I read it over and think I understand what's happening and it looks fine to me. Thanks for adding this!

mmlb

just a few minor nits the only one I'd really like to see fixed is the comment on startServersAndConnectClient

mmlb · 2022-01-06T20:31:00Z

grpc-server/grpc_server.go

+	var m sync.RWMutex
 	s := &Server{
 		log:            l,
 		hardwareClient: hc,
+		subLock:        &m,


Suggested change

var m sync.RWMutex

s := &Server{

log: l,

hardwareClient: hc,

subLock: &m,

s := &Server{

log: l,

hardwareClient: hc,

subLock: &sync.RWMutex{},

should work too and doesn't need a unnecessary variable.

mmlb · 2022-01-07T17:36:45Z

grpc-server/grpc_helpers_test.go

+	zt.mu.RUnlock()
+}
+
+// serveAndDial starts 2 grpc services.


should be startServersAndConnectClient

And the rest of the comment updated too. 😄

mmlb · 2022-01-07T17:37:30Z

grpc-server/subscribe_test.go

+	})
+
+	t.Run("good", func(t *testing.T) {
+		// id := uuid.Must(uuid.NewRandom()).String()


should be deleted

mmlb · 2022-01-07T17:38:34Z

grpc-server/subscribe_test.go

+		data := map[string]string{}
+		value := fmt.Sprintf(`{"id": "%s", "ip": "%s"}`, id, id)
+		payload := fmt.Sprintf(`%s=%s`, id, value)
+		data[id] = value


could be:

Suggested change

data := map[string]string{}

value := fmt.Sprintf(`{"id": "%s", "ip": "%s"}`, id, id)

payload := fmt.Sprintf(`%s=%s`, id, value)

data[id] = value

value := fmt.Sprintf(`{"id": "%s", "ip": "%s"}`, id, id)

payload := fmt.Sprintf(`%s=%s`, id, value)

data := map[string]string{

id: value,

}

but not a big deal to change really

Fixes #69 Signed-off-by: Nahum Shalman <[email protected]>

This allows us to accept "bufconn" connections during testing without causing panics Signed-off-by: Nahum Shalman <[email protected]> Co-authored-by: Manuel Mendez <[email protected]>

grpc-server/subscribe_test.go

Signed-off-by: Nahum Shalman <[email protected]> Co-authored-by: Manuel Mendez <[email protected]>

mmlb

nshalman requested a review from mmlb January 6, 2022 20:16

nshalman self-assigned this Jan 6, 2022

nshalman force-pushed the nshalman/subscribe-fix-and-test branch 4 times, most recently from 2ae2cf5 to 6699895 Compare January 6, 2022 20:39

nshalman force-pushed the nshalman/subscribe-fix-and-test branch from 6699895 to f0ae94c Compare January 7, 2022 16:39

nshalman force-pushed the nshalman/subscribe-fix-and-test branch from f0ae94c to b149044 Compare January 7, 2022 17:26

tobert previously approved these changes Jan 7, 2022

View reviewed changes

mmlb suggested changes Jan 7, 2022

View reviewed changes

nshalman and others added 2 commits January 7, 2022 13:11

subLock needs to be initialized

88728c4

Fixes #69 Signed-off-by: Nahum Shalman <[email protected]>

Refactor client address parsing

e74436c

This allows us to accept "bufconn" connections during testing without causing panics Signed-off-by: Nahum Shalman <[email protected]> Co-authored-by: Manuel Mendez <[email protected]>

nshalman dismissed tobert’s stale review via 0bd1af9 January 7, 2022 18:11

nshalman force-pushed the nshalman/subscribe-fix-and-test branch from b149044 to 0bd1af9 Compare January 7, 2022 18:11

nshalman commented Jan 7, 2022

View reviewed changes

grpc-server/subscribe_test.go Outdated Show resolved Hide resolved

Create a Subscribe test

4b367ad

Signed-off-by: Nahum Shalman <[email protected]> Co-authored-by: Manuel Mendez <[email protected]>

nshalman force-pushed the nshalman/subscribe-fix-and-test branch from 0bd1af9 to 4b367ad Compare January 7, 2022 18:22

nshalman requested a review from mmlb January 7, 2022 18:23

mmlb approved these changes Jan 7, 2022

View reviewed changes

nshalman added the ready-to-merge Signal to Mergify to merge the PR. label Jan 7, 2022

mergify bot merged commit df55fcc into main Jan 7, 2022

mergify bot deleted the nshalman/subscribe-fix-and-test branch January 7, 2022 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Subscribe panic and add a test which would have caught it #74

Fix Subscribe panic and add a test which would have caught it #74

nshalman commented Jan 6, 2022 •

edited

Loading

codecov bot commented Jan 6, 2022 •

edited

Loading

nshalman commented Jan 7, 2022

mmlb commented Jan 7, 2022

nshalman commented Jan 7, 2022 •

edited

Loading

tobert left a comment

mmlb left a comment

mmlb Jan 6, 2022

nshalman Jan 7, 2022

mmlb Jan 7, 2022

nshalman Jan 7, 2022

nshalman Jan 7, 2022

mmlb Jan 7, 2022

nshalman Jan 7, 2022

mmlb Jan 7, 2022

nshalman Jan 7, 2022

mmlb left a comment

Fix Subscribe panic and add a test which would have caught it #74

Fix Subscribe panic and add a test which would have caught it #74

Conversation

nshalman commented Jan 6, 2022 • edited Loading

Description

Why is this needed

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

codecov bot commented Jan 6, 2022 • edited Loading

Codecov Report

nshalman commented Jan 7, 2022

mmlb commented Jan 7, 2022

nshalman commented Jan 7, 2022 • edited Loading

tobert left a comment

Choose a reason for hiding this comment

mmlb left a 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

mmlb left a comment

Choose a reason for hiding this comment

nshalman commented Jan 6, 2022 •

edited

Loading

codecov bot commented Jan 6, 2022 •

edited

Loading

nshalman commented Jan 7, 2022 •

edited

Loading