Skip to content

Support connection over Unix socket #18

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

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

Conversation

HugoDelval
Copy link

Fix #16

@HugoDelval
Copy link
Author

I've updated the PR to preserve retro-compatibility.
It introduces a new method for those who want to use something else than TCP for the communication protocol. Otherwise it defaults to TCP.

@arp242
Copy link

arp242 commented Dec 20, 2017

I was thinking that maybe it's better to change the New() function to something like:

type Dialer func(context.Context) (net.Conn, error)

func new (Dialer d) {
    return &Client{dialer: d}
}

And then it can be used with both TCP and Unix connections:

dialer := net.Dialer{Timeout: 20 * time.Second}.
New(func(ctx context.Context) (net.Conn, error) {
    conn := dialer.DialContext(ctx, "tcp", "17.0.0.1")
    conn.SetDeadline(time.Now().Add(dialer.Timeout))
    return conn, nil
})

The Client.dial() calls can be replaced with this. We can add two standard functions for easy usage:

func TCPDialer(addr string) Dialer {
    return func(ctx context.Context) (net.Conn, error) {
        // ...
    }
}

So you can connect with:

client := New(TCPDialer("127.0.0.1"))

What do you think?

@arp242
Copy link

arp242 commented Dec 20, 2017

btw another common use case that should work without a lot of effort is connecting over TLS/SSL.

@HugoDelval
Copy link
Author

It would look way better but it will break retro-compatibility. If that's not an issue then let's go for it !

@arp242
Copy link

arp242 commented Dec 20, 2017

Compatibility isn't an issue as far as I'm concerned. I don't think there are a lot of people using it, and there is no "1.0" (exactly so that I could change the connection logic).

@HugoDelval
Copy link
Author

Okay perfect, then I'll try to update the PR! :)

@HugoDelval
Copy link
Author

Is it close that what you had in mind?

@codecov
Copy link

codecov bot commented Dec 20, 2017

Codecov Report

Merging #18 into master will decrease coverage by 0.47%.
The diff coverage is 64.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
- Coverage   75.66%   75.19%   -0.48%     
==========================================
  Files           2        2              
  Lines         374      379       +5     
==========================================
+ Hits          283      285       +2     
- Misses         53       55       +2     
- Partials       38       39       +1
Impacted Files Coverage Δ
spamc.go 79.27% <100%> (+0.99%) ⬆️
api.go 70.96% <62.5%> (-1.76%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d1cc77...d5d275f. Read the comment docs.

@HugoDelval
Copy link
Author

Do you need the coverage check to pass?

@HugoDelval
Copy link
Author

FYI I've tested it on a personal project and it works 🎉

@arp242
Copy link

arp242 commented Dec 21, 2017

Thanks! Look good at a glance; I'll test and go over it later.

Hugo DELVAL and others added 3 commits December 22, 2017 00:31
- Some small documentation fixes.
- Unexport the BuildGenericDialer() function for now. I'm not sure how
  useful it is to have this exported.
- The Client.dial() function is rather redundant now, so remove that.
Hm, need to think about the API for a bit here. Can't say I particularly
like this.
api.go Outdated
func New(d Dialer) *Client {
return &Client{dialer: d}
}

// BuildGenericDialer is a generic method to build a Dialer (cf `New` method)
func BuildGenericDialer(addr string, proto string, timeout time.Duration) Dialer {
Copy link
Author

Choose a reason for hiding this comment

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

@Carpetsmoker This was useful because it provided a way to specify the timeout. Without it the 20sec timeout is imposed.

Copy link
Author

Choose a reason for hiding this comment

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

@Carpetsmoker any update on this PR? :)

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