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

Panic when checking res.StatusCode #2175

Closed
devigned opened this issue Jul 3, 2018 · 3 comments
Closed

Panic when checking res.StatusCode #2175

devigned opened this issue Jul 3, 2018 · 3 comments
Milestone

Comments

@devigned
Copy link
Member

devigned commented Jul 3, 2018

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference

When doing a GET to a resource group using the ARM v17 API, I received a panic when checking the response StatusCode. Since one must check the err and the http status code to tell if there has been a 404, I would never expect dereferencing the response would cause a panic.

You can reliably cause a panic if you set the HTTP_PROXY env variable to an address that fails in tcp dial as demonstrated below if you added the following code to check the response.

if group.Response.Response == nil {
	return nil, err
}
Failed to execute the refresh request. Error = 'Post https://login.microsoftonline.com/TENANT_ID/oauth2/token?api-version=1.0: proxyconnect tcp: dial tcp 127.0.0.1:8888: connect: connection refused'

There should be clear direction on how to handle a tcp dial error and/or a default value populated into the response struct.

@jhendrixMSFT
Copy link
Member

One of the principles for these APIs is that if the API receives an HTTP response then it will be propagated to the caller (barring bugs of course). For your example using an invalid proxy, the response field is nil because there was no response; while personally I feel this is more descriptive than populating a zero-initialized HTTP response I can see that having it could make error checking a bit easier.
The bigger problem as you point out is knowing how to deal with errors returned from an API. Right now the returned errors are pretty opaque which makes it hard to reason about them and since we wrap almost everything in an autorest.DetailedError we are left with minimal facilities (normally in this case you could type assert to net.Error and call Temporary() to see if the dialer reports this as transient [which sadly doesn't work correctly in all cases]).

@jhendrixMSFT jhendrixMSFT added this to the vNext milestone Jul 27, 2018
@jhendrixMSFT
Copy link
Member

This will be resolved in vNext following the same pattern we introduced in azblob; responses will be returned by reference e.g. (*FooThing, error) and if we receive no response we will return nil.

@jhendrixMSFT
Copy link
Member

Closing as the HTTP status code response helpers should simplify this case.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants