go-gin icon indicating copy to clipboard operation
go-gin copied to clipboard

fix panic risk in func Middleware

Open YueHonghui opened this issue 5 years ago • 2 comments

In func Middleware, c.Next would be panic, then span.Finish will not be executed.

//...
	return func(c *gin.Context) {
		carrier := opentracing.HTTPHeadersCarrier(c.Request.Header)
		ctx, _ := tr.Extract(opentracing.HTTPHeaders, carrier)
		op := opts.opNameFunc(c.Request)
		sp := tr.StartSpan(op, ext.RPCServerOption(ctx))
		ext.HTTPMethod.Set(sp, c.Request.Method)
		ext.HTTPUrl.Set(sp, opts.urlTagFunc(c.Request.URL))
		opts.spanObserver(sp, c.Request)

		// set component name, use "net/http" if caller does not specify
		componentName := opts.componentName
		if componentName == "" {
			componentName = defaultComponentName
		}
		ext.Component.Set(sp, componentName)
		c.Request = c.Request.WithContext(
			opentracing.ContextWithSpan(c.Request.Context(), sp))

		c.Next()

		ext.HTTPStatusCode.Set(sp, uint16(c.Writer.Status()))
		sp.Finish()
	}
//...

YueHonghui avatar Mar 07 '19 08:03 YueHonghui

@stuart-mclaren It seems much more difficult to fix panic risk with gin comparing to stdlib(https://github.com/opentracing-contrib/go-stdlib/pull/38). As you pointed out, gin's Writer.Status() will be 200 other than 500 when inner handler panicked. This is misleading, and not the same as golang's http implementation. Maybe I need to create an issue to gin's project first.

YueHonghui avatar May 16 '19 06:05 YueHonghui

Dears, any progress on the status code?

Just FYI, my current fix is just to leave the status code as it is: https://github.com/polyrabbit/go-gin/commit/e7a519105e6f2aec8c1cfc001c43278f043ec007

polyrabbit avatar Oct 07 '19 08:10 polyrabbit