search

openfaas / connector-sdk

SDK for connecting events to functions
MIT License
54 stars 25 forks source link

Use value semantics for sync.Mutex #43

Closed embano1 closed 4 years ago

embano1 commented 4 years ago

The Go convention is to use value semantics for sync.Mutex. sync.Mutex can be directly used without initialization.

When passing the controller to functions, pointer semantics are required (go vet will catch this). Since all methods on controller are pointer receivers, no further changes are required.

Signed-off-by: Michael Gasch mgasch@vmware.com

alexellis commented 4 years ago

Thanks for the interest Michael. Given limited time and no funding from VMware for this project, I'm curious what this is needed for beyond tinkering?

Whilst I'm happy to continue to donate my free time for VEBA, it may make sense to keep requests focused on core functionality that you need.

Also can you use the PR template which says how you tested the change amongst other things?

Thank you 🙏

embano1 commented 4 years ago

Thank you for your quick response Alex!

Given limited time and no funding from VMware for this project, I'm curious what this is needed for beyond tinkering?

I can't follow why you associate a perceived funding problem with a particular vendor in a pull request. I don't think we should discuss such matters here in PR scoped comments.

Whilst I'm happy to continue to donate my free time for VEBA, it may make sense to keep requests focused on core functionality that you need.

Again, nothing in particular for VEBA. It is also my free time I spend on trying to improve the SDK (which I've done in the past and I'm happy to continue in the future as I do like this implementation).

The reason I created this pull request was purely from a recent quick code review I did on the connector SDK after I had to upgrade from an older version. This is where I saw the use of pointer semantics for the mutex in the controller struct.

Even though from today's code perspective it does not make any difference whether we use *sync.Mutex or sync.Mutex, I thought this small PR could still provide value for the SDK:

1) It follows Go conventions where sync.Mutex in a struct is usually not of type * (see Go stdlib) 2) It prevents accidentally sharing the mutex when the controller would be passed by value to internal functions (this could happen in future when the SDK grows)

If this is not a reasonable change to the SDK, I'm totally fine. Feel free to close the PR then.

Also can you use the PR template which says how you tested the change amongst other things?

I checked before raising this PR, but neither can't find a PR template nor a contributing guide on which PRs are considered useful. PRs from other users also did not use a particular template.

This PR was tested successfully:

Thanks for taking the time to discuss this PR.