Skip to content

Commit

Permalink
MM-34000: Use non-epoll mode for TLS connections (mattermost#17172)
Browse files Browse the repository at this point in the history
* MM-34000: Use non-epoll mode for TLS connections

A *crypto/tls.Conn does not expose the underlying TCP connection
or even a File method to get the underlying file descriptor
like the way a *net/TCPConn does. Therefore the netpoll code would
fail to get the file descriptor.

Relevant issue here: mailru/easygo#3

It is indeed possible to use reflect black magic to get the unexported
member, but I have found unexpected errors during writing to the websocket
by getting the file descriptor this way. I do not want to spend time investigating
this especially since this is already released.

Once this is out, we can decide on the right way to fix this, most probably
by proposing to expose the File method or some other way.

https://mattermost.atlassian.net/browse/MM-34000

```release-note
Fix an issue where websockets wouldn't work with TLS connections.
In that case, we just fall back to the way it works for Windows machines,
which is to use a separate goroutine for reader connection.
```

* Ignore logging errors on non-epoll

On non-epoll systems, we needed to return an error
to break from the loop. But in that case, there is no
need to log the error

(cherry picked from commit 2743089)
  • Loading branch information
agnivade authored and mattermost-build committed Mar 18, 2021
1 parent 91761b4 commit b6597c8
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 12 deletions.
3 changes: 1 addition & 2 deletions api4/websocket.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package api4

import (
"net/http"
"runtime"
"time"

"github.com/gobwas/ws"
Expand Down Expand Up @@ -49,7 +48,7 @@ func connectWebSocket(c *Context, w http.ResponseWriter, r *http.Request) {
c.App.HubRegister(wc)
}

if runtime.GOOS == "windows" {
if !wc.Epoll() {
wc.BlockingPump()
} else {
go wc.Pump()
Expand Down
28 changes: 18 additions & 10 deletions app/web_conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ const (
webConnMemberCacheTime = 1000 * 60 * 30 // 30 minutes
)

var errNonEpollConnClose = errors.New("connection closed")

// WebConn represents a single websocket connection to a user.
// It contains all the necessary state to manage sending/receiving data to/from
// a websocket.
Expand All @@ -55,7 +57,7 @@ type WebConn struct {
send chan model.WebSocketMessage
sessionToken atomic.Value
session atomic.Value
isWindows bool
hasEpoll bool
endWritePump chan struct{}
pumpFinished chan struct{}
closeOnce sync.Once
Expand All @@ -78,7 +80,7 @@ func (a *App) NewWebConn(ws net.Conn, session model.Session, t i18n.TranslateFun
UserId: session.UserId,
T: t,
Locale: locale,
isWindows: runtime.GOOS == "windows",
hasEpoll: *a.Config().ServiceSettings.ConnectionSecurity == "" && runtime.GOOS != "windows",
endWritePump: make(chan struct{}),
pumpFinished: make(chan struct{}),
}
Expand All @@ -87,8 +89,7 @@ func (a *App) NewWebConn(ws net.Conn, session model.Session, t i18n.TranslateFun
wc.SetSessionToken(session.Token)
wc.SetSessionExpiresAt(session.ExpiresAt)

// epoll/kqueue is not available on Windows.
if !wc.isWindows {
if wc.hasEpoll {
wc.startPoller()
}
return wc
Expand All @@ -101,7 +102,7 @@ func (a *App) NewWebConn(ws net.Conn, session model.Session, t i18n.TranslateFun
func (wc *WebConn) Close() {
wc.closeOnce.Do(func() {
wc.WebSocket.Close()
if !wc.isWindows {
if wc.hasEpoll {
// This triggers the pump exit.
// If the pump has already exited, this just becomes a noop.
close(wc.endWritePump)
Expand Down Expand Up @@ -145,6 +146,11 @@ func (wc *WebConn) SetSession(v *model.Session) {
wc.session.Store(v)
}

// Epoll returns whether the websocket is eligible to use epoll or not.
func (wc *WebConn) Epoll() bool {
return wc.hasEpoll
}

// Pump starts the WebConn instance. After this, the websocket
// is ready to send messages.
// This is only used by *nix platforms.
Expand All @@ -156,7 +162,7 @@ func (wc *WebConn) Pump() {
close(wc.pumpFinished)
}

// BlockingPump is the Windows alternative of Pump.
// BlockingPump is the non-epoll alternative of Pump.
// It creates two goroutines - one for reading, another
// for writing.
func (wc *WebConn) BlockingPump() {
Expand Down Expand Up @@ -242,9 +248,9 @@ func (wc *WebConn) ReadMsg() error {
switch hdr.OpCode {
case ws.OpClose:
// Return if closed.
// We need to return an error for Windows to let the reader exit.
if wc.isWindows {
return errors.New("connection closed")
// We need to return an error for non-epoll systems to let the reader exit.
if !wc.hasEpoll {
return errNonEpollConnClose
}
return nil
case ws.OpPong:
Expand Down Expand Up @@ -274,7 +280,9 @@ func (wc *WebConn) readPump() {

for {
if err := wc.ReadMsg(); err != nil {
wc.logSocketErr("websocket.read", err)
if err != errNonEpollConnClose {
wc.logSocketErr("websocket.read", err)
}
return
}
}
Expand Down

0 comments on commit b6597c8

Please sign in to comment.