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

Refactor to use Decoder struct #23

Merged
merged 1 commit into from Jun 20, 2016

Conversation

Projects
None yet
2 participants
@cgilling
Copy link
Contributor

cgilling commented Jun 19, 2016

All the main logic has been moved to functions on the Decoder
struct and the package level functions merely construct and use
a global detector that is protected by a mutex so that is is safe
for concurrent use. This will solve #20

NOTE: previously I had added TypeByFileHandle so that detection can be done on a *os.File, but this had troubles on linux, so I removed it.

@cgilling cgilling changed the title Refactor to be a Detector struct + addd TypeByFileHandle Refactor to be a Detector struct + add TypeByFileHandle Jun 19, 2016

@cgilling

This comment has been minimized.

Copy link
Contributor Author

cgilling commented Jun 19, 2016

Hmmm I did my testing on os x and everything worked locally, I will have to do some testing on linux to see why this isn't working

@cgilling cgilling force-pushed the cgilling:add_osfile_interface branch from 5f4babf to c01c449 Jun 19, 2016

@cgilling cgilling changed the title Refactor to be a Detector struct + add TypeByFileHandle Refactor to be a Detector struct Jun 19, 2016

@cgilling

This comment has been minimized.

Copy link
Contributor Author

cgilling commented Jun 19, 2016

I found that for some reason passing the os.File.Fd() to magic_descriptor resulted in the files descriptor being closed on linux, so I backed out that change, to get the same functionality one would just call TypeByFile passing in https://golang.org/pkg/os/#File.Name

@cgilling

This comment has been minimized.

Copy link
Contributor Author

cgilling commented Jun 19, 2016

Based on the comments if see in #21, it might be worth explaining some of the motivations here:

  • I would like to use this library in some server code that has a high level of concurrency and would therefore like to be able to have multiple handles to libmagic so that I can do mime detection concurrently if needed. The current way the library works does not allow for this and that is why I introduced the Detector struct that itself is not safe for concurrent use, but one can have multiple of them in use concurrently
  • I do think that it is not ideal to have the package level functions not be safe for concurrent use. From my experience the large majority of package level functions are safe for concurrent use. As an example I think of the rand standard package. It's package level functions are safe for concurrent use because they are protected by a mutex, but if you create a rand.Source of your own, it is not safe for concurrent use.
  • In contrast to the implementation in #21 this does not break the current API of this package.
// call Close when they are finished using the package. This must
// be called before any of the package level functions.
func Open(flags Flag) error {
detMu.Lock()

This comment has been minimized.

@rakyll

rakyll Jun 19, 2016

Owner

I am not sure this is the expected behavior. I would rather move the package level functions or return an error if there is an already opened db.

This comment has been minimized.

@cgilling

cgilling Jun 19, 2016

Author Contributor

sure, that makes sense to me

This comment has been minimized.

@cgilling

cgilling Jun 19, 2016

Author Contributor

I changed this in the latest commit to return an error instead


// TypeByFile calls TypeByFile on the global Detector. This is safe for
// concurrent use with the other package level TypeBy* functions.
func TypeByFile(filePath string) (string, error) {

This comment has been minimized.

@rakyll

rakyll Jun 19, 2016

Owner

Can we rename the filePath to filename while we are there? Maybe TypeByFile to TypeByFilename too.

This comment has been minimized.

@cgilling

cgilling Jun 19, 2016

Author Contributor

I can do this, I was just using what was here already. I do wonder if we want to change TypeByFile -> TypeByFilename while as its probably more correct in the name, it will break backwards compatibility. I'll leave the function name as it is for now until I hear from you on whether or not you want to break the backwards compatibility.

"unsafe"
)

var db C.magic_t
// Detector allows for doing mimetype detection using libmagic.
type Detector struct {

This comment has been minimized.

@rakyll

rakyll Jun 19, 2016

Owner

s/Detector/Decoder.

@rakyll

This comment has been minimized.

Copy link
Owner

rakyll commented Jun 19, 2016

I left a few comments mainly about the organization of the code.

@cgilling cgilling changed the title Refactor to be a Detector struct Refactor to be a Decoder struct Jun 19, 2016

@cgilling cgilling changed the title Refactor to be a Decoder struct Refactor to use Decoder struct Jun 19, 2016

// detMu and det are used for the package level functions.
var (
detMu sync.Mutex
det *Decoder

This comment has been minimized.

@rakyll

rakyll Jun 20, 2016

Owner

At this point, we need to rename this to decoder or similar. Same for the mutex naming.

detMu.Lock()
defer detMu.Unlock()
if det != nil {
return errors.New("Open called twice without first calling Close")

This comment has been minimized.

@rakyll

rakyll Jun 20, 2016

Owner

Use always lower case messages.

errors.New("cannot open; magic mime db is already open") is enough.

Error messages are what end users see, so we shouldn't include any development-related suggestions.

@rakyll

This comment has been minimized.

Copy link
Owner

rakyll commented Jun 20, 2016

LGTM after a few nits.

create Decoder struct and make package funcs concurrent safe
All the main logic has been moved to functions on the Decoder
struct and the package level functions merely construct and use
a global decoder that is protected by a mutex so that is is safe
for concurrent use.

@cgilling cgilling force-pushed the cgilling:add_osfile_interface branch from 90ff104 to 896da00 Jun 20, 2016

@cgilling

This comment has been minimized.

Copy link
Contributor Author

cgilling commented Jun 20, 2016

Thanks for the quick feedback.

I addressed the two remaining issues and have rebased it down to one commit with the commit messages changes to represent the name change from Detector -> Decoder

@rakyll rakyll merged commit d05be4c into rakyll:master Jun 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rakyll

This comment has been minimized.

Copy link
Owner

rakyll commented Jun 20, 2016

Thanks much!

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