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

Client connection cleaner #1098

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

chhquan
Copy link

@chhquan chhquan commented Sep 11, 2021

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 implements resourceClean can register to the global cleaner.

@moredure
Copy link
Contributor

I love this idea

@moredure
Copy link
Contributor

moredure commented Sep 11, 2021

@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
}

@moredure
Copy link
Contributor

moredure commented Sep 11, 2021

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.

@moredure
Copy link
Contributor

moredure commented Sep 11, 2021

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Store client

The statement below is already super clear, it doesn't need a comment like this.

if client == nil {
return
}
// Check if init
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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)
Copy link
Collaborator

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 {
Copy link
Collaborator

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.

Copy link
Contributor

@tylitianrui tylitianrui left a 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() {
Copy link
Contributor

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()
}

Copy link
Collaborator

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.

@chhquan
Copy link
Author

chhquan commented Sep 26, 2021

@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 {
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants