-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Client connection cleaner #1098
base: master
Are you sure you want to change the base?
Conversation
I love this idea |
@chhquan Maybe change this func (c *Client) hasResource() bool {
c.mLock.Lock()
defer c.mLock.Unlock()
if len(c.m) > 0 || len(c.ms) > 0 {
return true
}
return false
} to this func (c *Client) hasResource() bool {
c.mLock.Lock()
defer c.mLock.Unlock()
return len(c.m) > 0 || len(c.ms) > 0
} |
And in case !c.connsCleanerRun && !connectionClose new connection is connectionClosed we do not need to count it as a connection, maybe since we should no register such a connections in connectionsCleaner and in some cases if all connections across fasthttp.Client will be connectionClosed, maybe we don't need connectionsCleaner goroutine running. |
I mean replace this if c.connsCount == 1 {
startCleaner = true
} maybe with this, not sure regarding connectionCounting maybe skip increasing connection counter in case connectionClose if c.connsCount > 0 && !connectionClose {
startCleaner = true
} to avoid creating connectionCleaner for one_time_requests-only client |
// Check if init | ||
c.init() | ||
|
||
// Store client |
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.
// Store client |
The statement below is already super clear, it doesn't need a comment like this.
if client == nil { | ||
return | ||
} | ||
// Check if init |
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.
// Check if init |
Not really a check and not really a needed comment.
// Clean Client when the client has no resource. | ||
func (c *clientCleaner) cleanClient() { | ||
// Find all clients | ||
allClients := make([]resourceClean, 0) |
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.
What is this slice for? Merge the loops below so this isn't needed anymore.
|
||
// Resource Clean interface. | ||
// resourceClean can register into clientCleaner for cleaning resources. | ||
type resourceClean interface { |
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.
Maybe call it cleanableClient
? Either you use resource
everywhere and make it all generic. Or use client
and make it specific for clients.
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.
it can avoid mutex . sync.Once
is implemented by Mutex
|
||
// Initialize cleaner. | ||
func (c *clientCleaner) init() { | ||
c.initOnce.Do(func() { |
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.
I think if atomic.CompareAndSwapUint32(&o.done, 0, 1) )
is better
clientCleaner
type clientCleaner struct {
done int32
// clients to clean
clients sync.Map
}
method init:
func (c *clientCleaner) init() {
if atomic.CompareAndSwapUint32(&o.done, 0, 1) {
go c.cleaner()
}
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.
Good catch, sync.Once
is only really needed if what you are doing inside it is really required for the rest of the code to continue. This is not the case here, it's perfectly fine if the goroutine is started after calls to register
have completed already.
@moredure @erikdubbelboer @tylitianrui ok! i will change the code for your suggestion. |
func (c *Client) hasResource() bool { | ||
c.mLock.Lock() | ||
defer c.mLock.Unlock() | ||
if len(c.m) > 0 || len(c.ms) > 0 { |
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.
simplify return len(c.m) > 0 || len(c.ms) > 0
Client and HostClient register itself to cleaner to clean resource, instead of creating a goroutine. new interface
resourceClean
will help a client to clean resource. Client implementsresourceClean
can register to the global cleaner.