[v17] Add latency detector for desktop sessions (#52827)#54604
[v17] Add latency detector for desktop sessions (#52827)#54604probakowski wants to merge 3 commits intobranch/v17from
Conversation
* wip * add ping message and latency from desktop side * version * Fix backward compatibility * godocs * formatting * fix imports * formatting * formatting * log and gci * lint * Apply tooltip on the icon directly, instead of on the Menu component This fixes a problem where onMouseLeave in HoverTooltip wasn't called and the tooltip didn't disappear. * Use consistent spacing between top bar elements * updates from origin * e * e * lint * rework UI after merge * Rename fields in backend * prettier * Update web/packages/shared/components/DesktopSession/TopBar.tsx Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com> * review comments * fix ui * add env var to disable windows desktop "ping" * review comment * review comment * Update lib/web/desktop.go Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> * Refactor latency monitoring * fix spelling * remove monitorSessionLatency * gci * review comment * review comment * Update RFD and version * fix gaps --------- Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> Co-authored-by: Grzegorz Zdunek <grzegorz.zdunek@goteleport.com> Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com>
github-actions
Bot
added
backport
desktop-access
rdp
rfd
espadolini
left a comment
espadolini
left a comment
There was a problem hiding this comment.
The concurrency bugs around the handling of ping and latency report messages are likely a result of needless complexity, I would recommend taking a step back and rethinking of what should happen and when, and to try to make that happen with fewer moving parts.
| conn.Close() | ||
| } | ||
| } | ||
| if err := c.cfg.Conn.WriteMessage(m); err != nil { |
There was a problem hiding this comment.
This will unsynchronously write into an io.ReadWriteCloser - which is contractually not safe for concurrent write - that's set from a net.Conn, which is safe for concurrent writes from a concurrency standpoint, but with utterly meaningless results (if any of the concurrent writes is larger than one byte) since there's no telling if the underlying writes are split and in which order the bytes are going to be jumbled on the wire.
There was a problem hiding this comment.
The writer in question is *tls.Conn which is thread safe (internally guarded by mutex), so it wouldn't be a problem here. Could you check #54795 where I added same guard in tdp.Conn so any io.ReadWriteCloser can work here (I simplified other parts of code, too)
| errs.Go(func() error { | ||
| for { | ||
| select { | ||
| case msg := <-tdpMessagesToSend: |
There was a problem hiding this comment.
The goroutine running the monitorLatency callback can get stuck on tdpMessagesToSend when this loop exits.
|
Already merged in different PR |
3 participants
Backports #52827 to
branch/v17The differences between this and
masterversion are logging and fix for goroutines leak that I'll add to master in separate PRchangelog: Add latency detector for desktop sessions