Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suite.Require deadlock #1520

Closed
vitalyisaev2 opened this issue Dec 27, 2023 · 1 comment · Fixed by #1535
Closed

suite.Require deadlock #1520

vitalyisaev2 opened this issue Dec 27, 2023 · 1 comment · Fixed by #1535
Labels
bug pkg-suite Change related to package testify/suite

Comments

@vitalyisaev2
Copy link
Contributor

If require object hasn't been initialized before calling suite.Require method, it will be created on the base of *testing.T returned from suite.T() (code):

// Require returns a require context for suite.
func (suite *Suite) Require() *require.Assertions {
	suite.mu.Lock()
	defer suite.mu.Unlock()
	if suite.require == nil {
		suite.require = require.New(suite.T())
	}
	return suite.require
}

This will result in a deadlock because suite.T() wants to acquire the lock that has been already acquired:

// T retrieves the current *testing.T context.
func (suite *Suite) T() *testing.T {
	suite.mu.RLock()
	defer suite.mu.RUnlock()
	return suite.t
}
arjunmahishi added a commit to arjunmahishi/testify that referenced this issue Feb 16, 2024
As pointed out in issue stretchr#1520, if the suite is not initialised properly
(buy calling the Run function), then calling suite.Require() or
suite.Assert() will result in a deadlock.

This commit fixes that by panicking if the suite is not initialised
properly. This is justified because, the suite is intended to be
triggered in the right way. If the user does not do that, this panic will
nudge them in the right direction.

It has to be a panic because, at this point, we don't have access to any
testing.T context to gracefully call a t.Fail(). Also, these two
functions are not expected to return an error.

Fixes stretchr#1520
@arjunmahishi
Copy link
Collaborator

Thanks for reporting this @vitalyisaev2!

I've put up a PR for fixing this. If you have any thoughts about the approach, please comment on #1535

@arjunmahishi arjunmahishi added bug pkg-suite Change related to package testify/suite labels Feb 16, 2024
arjunmahishi added a commit to arjunmahishi/testify that referenced this issue Feb 18, 2024
As pointed out in issue stretchr#1520, if the suite is not initialised properly
(buy calling the Run function), then calling suite.Require() or
suite.Assert() will result in a deadlock.

This commit fixes that by panicking if the suite is not initialised
properly. This is justified because, the suite is intended to be
triggered in the right way. If the user does not do that, this panic will
nudge them in the right direction.

It has to be a panic because, at this point, we don't have access to any
testing.T context to gracefully call a t.Fail(). Also, these two
functions are not expected to return an error.

Fixes stretchr#1520
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug pkg-suite Change related to package testify/suite
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants