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

http.Request.Context() false positive #3

Open
vikstrous2 opened this issue Dec 13, 2021 · 16 comments
Open

http.Request.Context() false positive #3

vikstrous2 opened this issue Dec 13, 2021 · 16 comments
Labels
question Further information is requested

Comments

@vikstrous2
Copy link

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
Copy link
Owner

kkHAIKE commented May 15, 2022

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

@kkHAIKE kkHAIKE added the question Further information is requested label May 15, 2022
@fxrlv
Copy link

fxrlv commented Aug 25, 2022

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
Copy link
Owner

kkHAIKE commented Aug 25, 2022

fixed, wait new ci release
6a615a9

@ldemailly
Copy link

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
Copy link

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

@kkHAIKE
Copy link
Owner

kkHAIKE commented Sep 8, 2022

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

// 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
Copy link

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
Copy link
Owner

kkHAIKE commented Sep 9, 2022

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

@kkHAIKE
Copy link
Owner

kkHAIKE commented Sep 10, 2022

@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
Copy link

ldemailly commented Sep 18, 2022

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
Copy link
Owner

kkHAIKE commented Sep 19, 2022

new version can mark it automatically.


add the new tag to mark it serverside request force

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

@ldemailly
Copy link

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
Copy link

chancez commented Mar 30, 2023

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
Copy link
Owner

kkHAIKE commented Mar 31, 2023

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
Copy link

chancez commented Mar 31, 2023

@kkHAIKE awesome, that worked. Thanks.

@okhowang
Copy link

How about some third-party http library, for example gin

r.GET("/ping", func(c *gin.Context) {
   doSomething(c.Request.Context())
    c.JSON(http.StatusOK, gin.H{
      "message": "pong",
    })
  })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

6 participants