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

Reset expired connections #892

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

iamluc
Copy link
Contributor

@iamluc iamluc commented Feb 6, 2023

Summary

Ensure a connection is not used when its ConnMaxLifetime is exceeded

Currently, the ConnMaxLifetime is only checked in the release method. So a connection may be valid when added to the "idle" pool, but expired when "acquired".

In our application, the TCP socket is only alive for a few minutes, and I don't know why, but the isBad() method does not detect any issues.
But then when a query is sent to the ClickHouse server, we end up with broken pipe or EOF errors

@CLAassistant
Copy link

CLAassistant commented Feb 6, 2023

CLA assistant check
All committers have signed the CLA.

@jkaflik
Copy link
Contributor

jkaflik commented Feb 7, 2023

@iamluc, thanks for your submission. I will look and check if this is reproducible in a simple test.

BTW please sign CLA

@jkaflik jkaflik self-assigned this Feb 7, 2023
@jkaflik jkaflik added the bug label Feb 7, 2023
@jkaflik
Copy link
Contributor

jkaflik commented Feb 7, 2023

I don't think it's something I can get easily reproducible in tests.
@iamluc, my opinion is it would be better to check lifetime inside isBad:

func (c *connect) isBad() bool {
	switch {
	case c.closed:
		return true
	}

	if time.Since(c.connectedAt) >= c.opt.ConnMaxLifetime {
		return true
	}

	if err := c.connCheck(); err != nil {
		return true
	}
	return false
}

@iamluc
Copy link
Contributor Author

iamluc commented Feb 8, 2023

Thanks @jkaflik
PR updated with your suggestion and CLA signed!

Note: the changes in the import section are done automatically by the Golang plugin of VSCode. If it bothers you, I can revert this part

@jkaflik
Copy link
Contributor

jkaflik commented Feb 8, 2023

@iamluc thanks! imports change is fine. let me wait for green tests and we can merge it.

@jkaflik jkaflik merged commit 0753b78 into ClickHouse:main Feb 8, 2023
@iamluc iamluc deleted the reset-expired-connections branch February 9, 2023 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants