Skip to content

Don't pass around a pointer to service.#209

Open
jholdstock wants to merge 2 commits into
decred:masterfrom
jholdstock:no-pointer
Open

Don't pass around a pointer to service.#209
jholdstock wants to merge 2 commits into
decred:masterfrom
jholdstock:no-pointer

Conversation

@jholdstock
Copy link
Copy Markdown
Member

@jholdstock jholdstock commented May 26, 2026

It makes more sense for the functions to be methods of the service, rather than accepting the service as a param.

It makes more sense for the functions to be methods of the service,
rather than accepting the service as a param.
Copy link
Copy Markdown
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Nice update overall. It is cleaner to have the methods on the type and more customary to use a short name for receiver.

Comment thread service.go
service.Mutex.RLock()
vsp = service.Vsps[url]
service.Mutex.RUnlock()
s.Mutex.RLock()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be better to just take the vsp as a 2nd param. Then you wouldn't have to take the mutex again here. It looks like it's only called from the loop in vspData which already holds the mutex.

func (s *Service) vspStats(url string, vsp Vsp) error {
	infoURL := fmt.Sprintf("https://%s/api/v3/vspinfo", url)
	...
}
func (s *Service) vspData() {
	...
	for url, vsp := range s.Vsps {
		go func(url string, vsp Vsp) {
			defer wg.Done()
			err := s.vspStats(url, vsp)
			...
		}(url, vsp)
	...
}

Comment thread service.go
for url := range service.Vsps {
func (s *Service) vspData() {
var wg sync.WaitGroup
s.Mutex.RLock()
Copy link
Copy Markdown
Member

@davecgh davecgh May 26, 2026

Choose a reason for hiding this comment

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

By the way, it doesn't really matter, but you could shorten all of these mutex accesses by just embedding the mutex in the service since the mutex is already exported anyway.

e.g.

type Service struct {
	// the http client
	HTTPClient *http.Client
	// the http router
	Router *http.ServeMux

	// Data cached by the service, protected by a mutex.
	sync.RWMutex
	Vsps      vspSet
	...

Then:

s.RLock()
...
s.RUnlock()

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants