Skip to content
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

Change NewConvenientClient to call NewClient #12

Closed

Conversation

edwinmo-splunk
Copy link

Without this change, the following nil dereference occurs due to how Client in NewConvenientClient is created:

client: &{Client:{Token: CustomerName:splunk httpclient:0xc420086db0 transport:<nil> verbose:false}}

Program received signal SIGSEGV, Segmentation fault.
0x00000000004cebf1 in net/http.(*Transport).RoundTrip (t=0x0, req=0xc4200f00f0, ~r1=0x4,
    ~r2=...) at /usr/local/go/src/net/http/transport.go:307
307             t.nextProtoOnce.Do(t.onceSetNextProtoDefaults)

I've only recently (the last day or two) picked up Golang. If something is not up to snuff, please let me know.

@nesv
Copy link
Owner

nesv commented Mar 31, 2017

Hi @edwinmo-splunk! Sorry for having taken so long to take a peek at this. I'll look at it right now.

@nesv
Copy link
Owner

nesv commented Mar 31, 2017

@edwinmo-splunk Could you give me a little more context around this issue? How are you using ConvenientClient so that it results in a SIGSEGV, like this?

Also, is this on Windows? I ask, because I've yet to see a crash like that on Linux, FreeBSD, OpenBSD, or macOS; normally, if something crashes, or panics, there would be a stack trace of sorts printed out.

EDIT: I came across this bug, and I'm wondering, are you are noticing this issue on a long-running application? Also, are you using Go 1.8, or an earlier version?

@edwinmo-splunk
Copy link
Author

Hey @nesv, great to hear back from you!

This is not on Windows, but on Linux (Mint 17.3). The context in which this library is used is through Terraform (here: https://github.com/hashicorp/terraform/blob/master/builtin/providers/dyn/resource_dyn_record.go#L103). The version that Terraform is currently using is behind master. I was simply unable to get Terraform working with Dynect due to hashicorp/terraform#10607. I decided to monkey-patch the version to master here and ran into the issue this ticket solves. Solving this problem (rather than issue#10607 in Terraform) was easier.

I've unfortunately lost the full stack trace as I haven't come back to this problem after I got my POC working for my job (w/ this change). However the gist of it (from what I understood) was that ConvenientClient was being initialized without a transport object (this can be seen in my original post above ^). This was causing seg faults.

I can come back to this once (if) I come back to this POC, but as of now this is all the info I can provide. If there's another solution to the transport=nil problem, I'd love to hear it. Thanks!

@nesv
Copy link
Owner

nesv commented Apr 26, 2017

The only thing I would recommend, and this is really just based on personal style more than anything else, is rather than just calling NewClient() with a deferencing pointer, could your changes do the following:

...
httpclient: &http.Client{Transport: &http.Transport{}},

Once that's done, I will happily merge this pull request in. 😄

@nesv nesv self-assigned this Apr 26, 2017
@nesv nesv closed this in 878afd0 Jun 5, 2017
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