http.Request.Context() false positive

[vikstrous2]

I have code that looks like:

import (
	"net/http"

	"go.opencensus.io/trace"
)

func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
	defer func() {
		span := trace.FromContext(r.Context())
		_ = span
	}()

	h.wrappedHandler.ServeHTTP(w, r)
}

I get this error:

file.go:71:28: Non-inherited new context, use function like `context.WithXXX` instead (contextcheck)
                span := trace.FromContext(r.Context())
                                         ^

[kkHAIKE]

can't reproduce in my PC. add some info plz. 😊

[fxrlv]

package main

import (
	"context"

	"github.com/openzipkin/zipkin-go"
)

func main() {
	foo(context.Background())
}

func foo(ctx context.Context) {
	_ = WithTrace(nil)
}

func WithTrace(tracer *zipkin.Tracer) func(context.Context) context.Context {
	return func(ctx context.Context) context.Context {
		parent := zipkin.SpanOrNoopFromContext(
			ctx,
		)

		span := tracer.StartSpan("span",
			zipkin.Parent(parent.Context()),
		)

		_ = span
		return ctx
	}
}

main.go:14:15: Function `WithTrace->WithTrace$1` should pass the context parameter (contextcheck)
	_ = WithTrace(nil)

[kkHAIKE]

fixed, wait new ci release
6a615a9

[ldemailly]

I also get false positive with code that takes context from incoming http request and sets it into new requests

I got contextcheck from golangci-lint has version 1.49.0

faq I suppose but can I make it use latest or at least test/confirm if it's fixed?

[ldemailly]

the nolint I think shouldn't be needed (but may totally be wrong about) are fortio/fortio@73c9268#diff-dd948e8af77fe6765332b96b0e84a4f12554c233866ce4ef85731b7016dc572b

[kkHAIKE]

no.. is not use like normal nolint from ci..
it add to the last function doc comment like

contextcheck/testdata/src/a/a.go

Lines 112 to 115 in 5cf2741

	// nolint: contextcheck
	func f14(w http.ResponseWriter, r *http.Request, err error) {
	f8(r.Context(), w, r)
	}

i guess ci version 1.49.1 may have this fix

[ldemailly]

Sorry I wasn't clear. I meant not to ask how to disable a check, that's working fine with golangci-lint

I meant to ask why is the code I linked being flagged, and I just check with the latest code and it still is

/Users/dl/github/fortio/fortio/fhttp/http_forwarder.go:122:24: Function `TeeSerialHandler->do->setupRequest->MakeSimpleRequest->makeMirrorRequest->CopyHeaders` should pass the context parameter
/Users/dl/github/fortio/fortio/fhttp/http_forwarder.go:125:26: Function `TeeParallelHandler->TeeParallelHandler$1->setupRequest->MakeSimpleRequest->makeMirrorRequest->CopyHeaders` should pass the context parameter
/Users/dl/github/fortio/fortio/fhttp/http_server.go:424:26: Function `MakeSimpleRequest` should pass the context parameter

(btw I don't know how to read that TeeSerialHandler->do->setupRequest->MakeSimpleRequest->makeMirrorRequest->CopyHeaders)

[kkHAIKE]

it's seem have some trouble..i will fix later

[kkHAIKE]

@ldemailly bug fixed..and add new tag for set a func is http handler for analyse

i add tag to 3 func: makeMirrorRequest, MakeSimpleRequest, Run.

and attach new linter output:

fhttp/http_server.go:472:24: Function `NewClient->NewFastClient->resolve->Resolve->ResolveByProto->Resolve->ResolveByProto` should pass the context parameter (contextcheck)
	client, _ := NewClient(opts)
	                      ^
rapi/restHandler.go:357:31: Function `RunGRPCTest` should pass the context parameter (contextcheck)
		res, err = fgrpc.RunGRPCTest(&o)
		                            ^
rapi/restHandler.go:367:34: Function `RunTCPTest->NewTCPClient->ResolveDestination->TCPResolveDestination->ResolveDestinationInternal->ResolveByProto` should pass the context parameter (contextcheck)
		res, err = tcprunner.RunTCPTest(&o)
		                               ^
rapi/restHandler.go:377:34: Function `RunUDPTest->NewUDPClient->UDPResolveDestination->ResolveDestinationInternal->ResolveByProto` should pass the context parameter (contextcheck)
		res, err = udprunner.RunUDPTest(&o)
		                               ^
rapi/restHandler.go:385:31: Function `RunHTTPTest` should pass the context parameter (contextcheck)
		res, err = fhttp.RunHTTPTest(&o)
		                            ^

[ldemailly]

I tried the latest version, can you explain why:

func MakeSimpleRequest(url string, r *http.Request, copyAllHeaders bool) *http.Request {
	req, err := http.NewRequestWithContext(r.Context(), http.MethodGet, url, nil)
...

makes it say Function MakeSimpleRequest should pass the context parameter when called. Shouldn't passing the http.Request be enough and a good pattern to carry context?

[kkHAIKE]

new version can mark it automatically.

---

add the new tag to mark it serverside request force

// @contextcheck(req_has_ctx)
func MakeSimpleRequest(...

[ldemailly]

I am trying to ask and suggest if not that context check understands, without needing any special annotation that it is common to transfer/carry a context from http.Request

[chancez]

This also occurs with gRPC stream interceptors:

func StreamServerInterceptor(opts QueryInfoOptions) grpc.StreamServerInterceptor {
	return func(srv interface{}, stream grpc.ServerStream, _ *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
		ctx := stream.Context()
		...
        }
}

[kkHAIKE]

This also occurs with gRPC stream interceptors:

func StreamServerInterceptor(opts QueryInfoOptions) grpc.StreamServerInterceptor {
	return func(srv interface{}, stream grpc.ServerStream, _ *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
		ctx := stream.Context()
		...
        }
}

This case is too rare. Perhaps it would be better to wrap it in a function call.

func StreamServerInterceptor(opts QueryInfoOptions) grpc.StreamServerInterceptor {
	return func(srv interface{}, stream grpc.ServerStream, _ *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
                wrapcall(stream.Context(), srv, stream, handler)
        }
}

[chancez]

@kkHAIKE awesome, that worked. Thanks.