protocol icon indicating copy to clipboard operation
protocol copied to clipboard

Improvement suggestion for error handling

Open yeldiRium opened this issue 1 year ago • 0 comments

Hi there,

thanks for making this great package. It aids me a lot in building my own language server and I like it quite a bit.

However I have an improvement suggestion. I am currently trying to wrap the lps server handler to add telemetry. With web servers and other kinds of servers you would usually do this by wrapping the handler and recording some information about its execution. This currently looks like this, where innerHandler is a jsonrpc2.Handler as returned by protocol.ServerHandler(...).

func WrapInTelemetry(telemetry *Telemetry, innerHandler jsonrpc2.Handler) jsonrpc2.Handler {
	wrappedHandler := func(ctx context.Context, reply jsonrpc2.Replier, req jsonrpc2.Request) error {
		ctx, span := telemetry.Tracer.Start(ctx, req.Method())
		defer span.End()

		SetGlobalAttributes(span)

		err := innerHandler(ctx, reply, req)
		if err != nil {
			telemetry.logger.Sugar().Errorf("error occured and is being sent to telemetry!")
			span.SetStatus(codes.Error, "handler failed")
			span.RecordError(err)
			return err
		}

		span.SetStatus(codes.Ok, "handler succeeded")
		return nil
	}

	return wrappedHandler
}

Now I'm running into a problem: The inner handler does not propagate errors.

This is due the behavior in serverDispatch, where all of the handlers handle errors by sending them to the client - which makes sense - but then not propagating the error, but only returning an error to the server, if sending the reply to the client failed.

This is probably done to keep the error handling localized and not expose errors to the server that can be dealt with by informing the client about them. However, if the error can't be handled by telling the client about it - and I have some of these kinds of errors - then the server needs to be able to at least know about them and log them.

Do you think there is a way to change this behavior without breaking everything? Or is there an alternative way for my wrapper to know that an error occured?

yeldiRium avatar Jan 20 '25 09:01 yeldiRium