Mutex goroutine #112
Mutex goroutine #112
Conversation
Separated lease file storage using another goroutin Signed-off-by: ngadre-juniper <ngadre@juniper.net>
Signed-off-by: ngadre-juniper <ngadre@juniper.net>
Signed-off-by: ngadre-juniper <ngadre@juniper.net>
ngadre-juniper
force-pushed
the
mutex_goroutine
branch
from
26200b7
to
a07d82b
Compare
There was a problem hiding this comment.
Thanks for the PR!
I'm ok with adding the lock; However moving the file storage to an async context requires more discussion (see below).
Can you separate these 2 ideas in 2 PRs ? This way we can merge one quickly and have the required design discussion for the second.
This raises an important observation that we need to think and define an interface for performing deferred work that doesn't block the request, but this current solution isn't enough unfortunately:
- Obviously it would need to be implemented for both v4 and v6
- Adding arguments to all the handlers is possible but we need to explore any alternative.
- Any modification breaks API so while not a huge deal at this point for the software, it's still a pain that requires all the plugins to be modified. As you can see here, you forgot to update a couple plugins and the tests fail to build
- Also functions with more than a handful of arguments become unwieldy fairly quickly, especially when they are widely used or in interfaces.
Can you rebase the stickler-ci commit into yours so the signoff is correct please ? Or since it only seems to contain gofmt fixes, just run gofmt on the modified files and commit that
| @@ -2,11 +2,14 @@ | |||
| // This source code is licensed under the MIT license found in the | |||
| // LICENSE file in the root directory of this source tree. | |||
|
|
|||
| // Copyright (c) 2020, Juniper Networks, Inc. All rights reserved. | |||
There was a problem hiding this comment.
It's not "all rights reserved", since it's under the MIT license.
As for the added copyright holder notice, imo this is covered under "the CoreDHCP Authors"/the DCO/the git history, and unwanted so I'd rather the line be removed entirely
| record, ok := Recordsv4[req.ClientHWAddr.String()] | ||
| if !ok { | ||
| log.Printf("MAC address %s is new, leasing new IPv4 address", req.ClientHWAddr.String()) | ||
| rec, err := createIP(ipRangeStart, ipRangeEnd) | ||
| if err != nil { | ||
| log.Error(err) | ||
| recMutex.Unlock() |
There was a problem hiding this comment.
imo as soon as you're potentially unlocking in multiple codepaths, use defer to avoid any risk of forgetting an unlock or that a future refactor introduces a codepath without unlock
| f, err := os.OpenFile(filename, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) | ||
| if err != nil { | ||
| fileMutex.Unlock() |
There was a problem hiding this comment.
definitely use defer for those please
| var recMutex = &sync.Mutex{} | ||
| var fileMutex = &sync.Mutex{} |
There was a problem hiding this comment.
Move these declarations a bit lower, with the resources they're actually protecting, so it's clearer (around line 45, //various global variables
Additionally, no need to take the pointer here:
| var recMutex = &sync.Mutex{} | |
| var fileMutex = &sync.Mutex{} | |
| var recMutex sync.Mutex | |
| var fileMutex sync.Mutex |
This Pull Request fix for #110 as well as separating the lease file storage using a goroutine.