-
Notifications
You must be signed in to change notification settings - Fork 809
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Adds a small test to catch issues with deadlocks #5171
Adds a small test to catch issues with deadlocks #5171
Conversation
func (s *taskSuite) TestHandleErr_ErrMaxAttempts() { | ||
taskBase := s.newTestTask(func(task Info) (bool, error) { | ||
return true, nil | ||
}, nil) | ||
|
||
taskBase.criticalRetryCount = func(i ...dynamicconfig.FilterOption) int { return 0 } | ||
s.mockTaskInfo.EXPECT().GetTaskType().Return(0) | ||
assert.NotPanics(s.T(), func() { | ||
taskBase.HandleErr(errors.New("err")) | ||
}) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since a deadlock won't panic, I've done this in the past:
done := make(chan struct{})
go func() {
defer close(done)
deadlocky()
}()
select {
case <-done:
case <-time.After(time.Second): t.Error("timed out == deadlock")
}
fast enough to be practical, without waiting for a full minute or whatever for the normal whole-package test timeout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while I appreciate that makes for a better performing test, it... feels slightly overspecialised with the benefit of hindsight to look exclusively for deadlocks, rather than just checking the path entirely (though, I suppose it would catch a panic all the same).
It looks like there'll be a new testing.T
method SetTimeout(d time.Duration)
coming soon which should do exactly this and - imho, is a lot less noisy (golang/go#48157)
I don't feel super strongly about it, but I'm wary about one-off testing infra setups such as this in that their complexity might dissuade people to maintain / think about them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SetTimeout would be kinda nice, yea.
I mean more that this wasn't a panic symptom, and "does not panic" on its own is not all that useful of an assertion anyway, since a panicking test is already a test failure. The fact that it's in use seems to imply there's something panic-sensitive about the code in question, and AFAIK there is not.
Pull Request Test Coverage Report for Build 01871f21-31ef-46ee-afd1-732d9cae568d
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useful test either way, comments are just comments and not in any way meant as a requirement :)
commit 9d01035 Author: allenchen2244 <102192478+allenchen2244@users.noreply.github.com> Date: Wed Mar 29 20:50:38 2023 -0700 large workflow hot shard detection (#5166) Metrics for large workflows commit dd51c53 Author: David Porter <david.porter@uber.com> Date: Wed Mar 29 18:30:06 2023 -0700 fix build (#5180) commit 7b281c2 Author: David Porter <david.porter@uber.com> Date: Mon Mar 27 10:38:37 2023 -0700 Adds a small test to catch issues with deadlocks (#5171) * Adds a small test to catch issues with deadlocks commit f1e2476 Author: sonpham96 <sonpham1996@gmail.com> Date: Sat Mar 18 05:32:01 2023 +0700 Upgrade Golang base image to 1.18 to remediate CVEs (#5035) Co-authored-by: David Porter <david.porter@uber.com> commit 1519ace Author: charlese-instaclustr <76502507+charlese-instaclustr@users.noreply.github.com> Date: Fri Mar 17 22:11:27 2023 +0000 Fix type validation in configstore DC client value updating (#5110) * Remove misleading type check, Add more detailed log message * removing debugging logging * Handle nil update edge case --------- Co-authored-by: allenchen2244 <102192478+allenchen2244@users.noreply.github.com> Co-authored-by: Zijian <Shaddoll@users.noreply.github.com> commit a3e2774 Author: charlese-instaclustr <76502507+charlese-instaclustr@users.noreply.github.com> Date: Fri Mar 17 19:02:40 2023 +0000 Add Canary TLS support (#5086) * add support for TLS connections by Canary, add development config for Canary with TLS * update README to include new config option * remove testing config --------- Co-authored-by: David Porter <david.porter@uber.com> Co-authored-by: Shijie Sheng <shengs@uber.com> Co-authored-by: Zijian <Shaddoll@users.noreply.github.com> commit ff4eab2 Author: Shijie Sheng <shengs@uber.com> Date: Thu Mar 16 20:10:54 2023 -0700 [history] more cautious in deciding domain state to make decisions on dropping queued tasks (#5164) What changed? When domain cache returned entity not found error, don't drop queued tasks to be more conservative. Why? In cases when the cache is dubious, we shouldn't drop the queued tasks. commit 55a8d93 Author: neil-xie <104041627+neil-xie@users.noreply.github.com> Date: Thu Mar 16 14:18:35 2023 -0700 Add Pinot docker files, table config and schema (#5163) * Initial checkin for pinot config files commit 1304570 Author: Mantas Šidlauskas <mantass@netapp.com> Date: Thu Mar 16 15:20:29 2023 +0200 Set poll interval for filebased dynamic config if not set (#5160) * Set poll interval for filebased dynamic config if not set * update unit test commit 42a14b1 Author: Mantas Šidlauskas <mantass@netapp.com> Date: Thu Mar 16 10:49:21 2023 +0200 Elasticsearch: reduce code duplication (#5137) * Elasticsearch: reduce code duplication * address comments --------- Co-authored-by: Zijian <Shaddoll@users.noreply.github.com> commit cbf0d14 Author: bowen xiao <xbowen@uber.com> Date: Wed Mar 15 10:19:34 2023 -0700 fix samples documentation (#5088) commit ba19a29 Author: Mantas Šidlauskas <mantass@netapp.com> Date: Wed Mar 15 12:52:29 2023 +0200 Add ShardID to valid attributes (#5161) commit a25cba8 Author: Mantas Šidlauskas <mantass@netapp.com> Date: Wed Mar 15 10:56:50 2023 +0200 ES: single interface for different ES/OpenSearch versions (#5158) * ES: single interface for different ES/OpenSearch versions * make fmt commit e3ac246 Author: Ketsia <115650494+ketsiambaku@users.noreply.github.com> Date: Tue Mar 14 12:47:40 2023 -0700 added logging with workflow/domain tags (#5159) commit 9581488 Author: Ketsia <115650494+ketsiambaku@users.noreply.github.com> Date: Mon Mar 13 16:56:45 2023 -0700 Consistent query pershard metric (#5143) * added and update consistent query per shard metric * testing pershard metric * move sample logger into persistence metric client for cleaness * fix test * fix lint * fix test again * fix lint * sample logging with workflowid tag * added domain tag to logger * metric completed * addressing comments * fix lint * Revert "fix lint" This reverts commit 1e96944. * fix lint second attempt --------- Co-authored-by: Allen Chen <allenchen2244@uber.com>
It's quite easy to call the getters on tasks in this function and cause a deadlock. Ensuring complete coverage on this branch to prevent this.