Thank you for filing a gopls issue! Please take a look at the Troubleshooting section of the gopls Wiki page, and make sure that you have provided all of the relevant information here.

Thank you for reporting this issue @muirrn! I shall send a fix for this immediately.

Change https://golang.org/cl/190637 mentions this issue: internal/telemetry/export: add load-shedding for workQueue

Thanks for looking at this. It looks like your change would avoid the deadlock, but it would introduce substantial latency under heavy contention. There is also guaranteed loss of telemetry data whenever it hits the current deadlock case.

It seems to me that the telemetry worker needs to be rearchitected in a more fundamental way. I'm sure @ianthehat already has great ideas, but I'm going to say my idea anyway: we could accumulate all telemetry messages for a single request in memory, and then kick off a goroutine to send them after the request completes. This way the telemetry messages for a given request are still sent in order, but they are always handled asynchronously with respect to LSP requests. If there is some requirement that the telemetry messages be processed "live" then this obviously wouldn't work.

Thanks for looking at this. It looks like your change would avoid the deadlock, but it would introduce substantial latency under heavy contention. There is also guaranteed loss of telemetry data whenever it hits the current deadlock case.

Right, it implements load shedding as a best effort and then drops overflowing events as it is a circular buffer of sorts, but ensures no deadlocks.

and then kick off a goroutine to send them after the request completes. This way the telemetry messages for a given request are still sent in order, but they are always handled asynchronously with respect to LSP requests. If there is some requirement that the telemetry messages be processed "live" then this obviously wouldn't work.

Events are processed online but the suggested batching will mean that the global ordering of events to process will be lost. Also, when metrics are collected, we'd now have to include timestamps too.

Anyways, good thoughts and concerns @muirrn! We can discuss more as we work on this issue.

Change https://golang.org/cl/190737 mentions this issue: internal/telemetry: change concurrency model

I think the load shedding may cause other significant issues (incorrect counts in rare events would be very bad, but much worse is memory leaks caused by unmatched start/finish span pairs for instance)
The concurrency model was always temporary, it was the quickest thing to implement that would be adequate while I got the API right.
I have put up a CL that might take us a bit further, and fixes the deadlock, but the final design needs more thought.

Thank you for the discourse @muirrn and @ianthehat! Sure, for now we can the mutex as Ian has posted up.