* MM-39612: Make acquiring and removing connections atomic
The reconnect phase of a websocket was split into two parts:
one where we check if a connection with a given connectionID
exists or not. And second, where we remove that connection
and insert the new connection again in the index.
This would lead to a race where it would be possible
for 2 concurrent requests for the same connectionID to go through
which would lead to separate goroutines working on the same dead queue.
We simplify this by removing the connection from the index
in the check connection stage itself. And then just add that
during register phase.
And to distinguish between a fresh and an old connection, we add
a new field called reuseCount.
While here, we also cleanup some old comments and add more
in some places.
https://mattermost.atlassian.net/browse/MM-39612
```release-note
NONE
```
* remove unused method
```release-note
NONE
```
* race test
```release-note
NONE
```
- Make it a Server method.
- Pass Server instead of App.
- Remove global app instance from NewServer, rather
create local instances whenever needed.
- Remove App from Websocket router, and create dynamically
on every request.
- Remove HubStart and HubStop from App methods.
- Explicitly using s.Log instead of the global logger
to indicate dependency on Server. We could have passed the logger
explicitly but it doesn't look ideal.
```release-note
NONE
```
* Move Channels into App
In this PR, we make Channels as part of App
instead of Server. This is part of the transition period
of moving fields from Server to Channels.
For now, Channels contains Server. So the hierarchy is
App -> Channels -> Server.
And as a first step, we also move httpService to Channels.
```release-note
NONE
```
* Fixing another test
```release-note
NONE
```
* new method
```release-note
NONE
```
* MM-25516: Changed to byte slice instead of string for cluster messages
https://mattermost.atlassian.net/browse/MM-25116
Testing:
Manually tested.
Load-tested with Cluster Controller.
I looked into changing the serialization method to use msgpack,
but the ClusterMessage struct was mainly used for only 3 fields
which didn't lead to much of a CPU time improvement, whereas
actually led to more allocations using msgpack. Hence, I chose
to remain with JSON.
```
name old time/op new time/op delta
ClusterMarshal-8 3.51µs ± 1% 3.10µs ± 2% -11.59% (p=0.000 n=9+10)
name old alloc/op new alloc/op delta
ClusterMarshal-8 776B ± 0% 1000B ± 0% +28.87% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
ClusterMarshal-8 12.0 ± 0% 13.0 ± 0% +8.33% (p=0.000 n=10+10)
```
```release-note
Changed the field type of Data in model.ClusterMessage to []byte from string.
```
* Trigger CI
```release-note
NONE
```
* MM-32950: Reliable WebSockets: Basic single server
This PR adds reliable websocket support for a single server.
Below is a brief overview of the three states of a connection:
Normal:
- All messages are routed via web hub.
- Each web conn has a send queue to which it gets pushed.
- A message gets pulled from the queue, and before it
gets written to the wire, it is added to the dead queue.
Disconnect:
- Hub Unregister gets called, where the connection is just
marked as inactive. And new messages keep getting pushed
to the send queue.
If it gets full, the channel is closed and the conn gets removed
from conn index.
Reconnect:
- We query the hub for the connection ID, and get back the
queues.
- We construct a WebConn reusing the old queues, or a fresh one
depending on whether the connection ID was found or not.
- Now there is a tricky bit here which needs to be carefully processed.
On register, we would always send the hello message in the send queue.
But we cannot do that now because the send queue might already have messages.
Therefore, we don't send the hello message from web hub, if we reuse a connection.
Instead, we move that logic to the web conn write pump. We check if
the sequence number is in dead queue, and if it is, then we drain
the dead queue, and start consuming from the active queue.
No hello message is sent here.
But if the message does not exist in the dead queue, and the sequence number
is actually something that should have existed, then we set
a new connction id and clear the dead queue, and send a hello message.
The client, on receiving a new connection id will automatically
set its sequence number to 0, and make the sync API calls to manage
any lost data.
https://mattermost.atlassian.net/browse/MM-32590
```release-note
NONE
```
* gofmt
* Add EnableReliableWebSockets to the client config
* Refactoring isInDeadQueue
* Passing index to drainDeadQueue
* refactoring webconn
* fix pointer
* review comments
* simplify hasMsgLoss
* safety comment
* fix test
* Trigger CI
* Trigger CI
Co-authored-by: Devin Binnie <devin.binnie@mattermost.com>
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
Remote Cluster Service
- provides ability for multiple Mattermost cluster instances to create a trusted connection with each other and exchange messages
- trusted connections are managed via slash commands (for now)
- facilitates features requiring inter-cluster communication, such as Shared Channels
Shared Channels Service
- provides ability to shared channels between one or more Mattermost cluster instances (using trusted connection)
- sharing/unsharing of channels is managed via slash commands (for now)
* MM-21012: Revamp websocket implementation
We replace the old gorilla/websocket implementation with the
gobwas/ws library. The gorilla library was in maintenance mode
and had a high level API due to which we cannot use that for
situations where a large number of concurrent connections needs
to be supported.
The ws library is a very low-level library that allows us
to work with raw net.Conns. We make several improvements:
- We completely remove the reader goroutines, and instead
replace them with a manual epoll implementation which sends off
messages to be read when it receives any data on the connection.
This lets us scale to a much larger number of connections.
- The reader buffer is eliminated, because we directly read
from the connection now.
https://mattermost.atlassian.net/browse/MM-21012
```release-notes
Improved the websocket implementation by using epoll manually
to read from a websocket. As a result, the number of goroutines
is expected to go down by half.
```
* fix tests
* fix shadowing errors
* final changes
* windows support!
* Remove pointer to waitgroup
* Fix edge case
* Trigger CI
* Trigger CI
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
* MM-24972: make GetHubForUserId lock-less and zero-alloc
The hubs aren't meant to be modified after the server starts up.
But the key problem was the SetHubs method which would zero out
the underlying hubs slice.
This wasn't ideally necessary because an hub would only be stopped
once during server shutdown. However, this was required by a test
which would shutdown the hub twice. And since the hub shutdown isn't
idempotent, zeroing the slice would just skip over shutting down the hub again.
To improve the overall situation, we apply several optimizations.
- We use the new hash/maphash package which exposes Go runtime's
internal hash algorithms to be used as a package. This is much faster
than hash/fnv.
- We move around the initialization of the hub to happen before
the metrics server starts. This allows us to initialize the hub
before any of the hub elements are being accessed.
- To make the test run successfully, we do not call th.TearDown.
This is fine for a test, because anyways the test process would eventually stop
and relinquish the resources to the OS.
This allows us to completely remove any mutexes and thereby
we can remove all the methods and any edge-case checks related to
index being out of bounds. As a result, the fast path becomes
very straightforward and zero-alloc.
name old time/op new time/op delta
GetHubForUserId-8 116ns ± 1% 38ns ± 7% -67.22% (p=0.000 n=10+10)
name old alloc/op new alloc/op delta
GetHubForUserId-8 36.0B ± 0% 0.0B -100.00% (p=0.000 n=10+10)
name old allocs/op new allocs/op delta
GetHubForUserId-8 2.00 ± 0% 0.00 -100.00% (p=0.000 n=10+10)
Manually tested with some load testing and running Hub tests in -race mode.
* remove mutex
* incorporate review comments
* Removing some other fake apps
* More FakeApp removed
* Removing entirely FakeApp
* Fixing some tests
* Fixing get Cluster id from get plugin status
* Fixing failing tests
* Fixing tests
* Fixing test initialization for web
* Fixing InitServer for server tests
* Fixing InitServer for server tests
* Reverting go.sum and go.mod
* Removing unneded HTMLTemplates function in App layer
* Moving back some functions to its old place to easy the review
* Moving back some functions to its old place to easy the review
* Using the last struct2interface version
* Generating store layers
* Fixing merge problems
* Addressing PR comments
* Small fix
* Fixing app tests build
* Fixing tests
* fixing tests
* Fix tests
* Fixing tests
* Fixing tests
* Fixing tests
* Moving license to server struct
* Adding some fixes to the test compilation
* Fixing cluster and some jobs initialization
* Fixing some license tests compilation problems
* Fixing recursive cache invalidation
* Regenerating app layers
* Fix test compilation
Co-authored-by: mattermod <mattermod@users.noreply.github.com>
After registering the conn in the hub, we proceeded to send a
direct message to the user. We had changed it to send the direct message
in the same hub goroutine that handles the registration. This was the correct
behavior and fixes chances of having panics due to sending to closed channels.
However, often fixing something unearths some deeper underlying bug. This was
such a case :)
The issue was that register channel had a buffer size of 1. And we were sending
a direct message after registration. In the code to send direct message, we
were checking if the user has been registered or not, and if not, then skip it.
Therefore, since the register channel buffer was 1, it could very well be that
the select case would pick up the direct message send case first - in which
case it would not have been registered, and therefore no hello message would be sent.
The fix is to unbuffer the register and unregister channels. There does not seem
to be a valid reason to make these buffered channels. They are meant to be
synchronous operations, because the code following them assumes that the user
has been registered.
While here, we also remove all the time.Sleeps before waiting on the Response channel
because they are not required at all. Waiting on a channel is already blocking.
Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>
* MM-23800: remove goroutineID and stack printing
Each hub has a goroutineID which is calculated with a known hack.
The FAQ clearly explains why goroutines don't have an id:
https://golang.org/doc/faq#no_goroutine_id.
We only added that because sometimes the hub would be deadlocked and
having the goroutineID would be useful when getting the stack trace.
This is also problematic in stress tests because the hubs would
frequently get overloaded and the logs would unnecessarily have stack traces.
But that was in the past, and we have done extensive testing with
load tests and fuzz testing to smooth any rough edges remaining.
Including adding additional metrics for hub buffer size.
Monitoring the metrics is a better way to approach this problem.
Therefore, we remove these kludges from the code.
* Also remove deadlock checking code
There is no need for that anymore since
we are getting rid of the stack printing anyways.
Let's do a wholesale refactor and clean up the codebase.
* MM-23805: Refactor web_hub
This is a beginning of the refactoring of the websocket code.
To start off with, we unexport some methods and constants which did not
need to be exported. There are more remaining but some are out of scope for this PR.
The main chunk of refactor is to unexport the webconn send channel
which was the main cause of panics. Since we were directly sending
to the connection from various parts of the codebase, it would be possible
that the send channel would be closed and we could still send a message.
This would crash the server.
To fix this, we refactor the code to centralize all sending from the main
hub goroutine. This means we can leverage the connections map to check
if the connection exists or not, and only then send the message.
We also move the cluster calls to cluster.go.
* bring back cluster code inside hub
* Incorporate review comments
* Address review comments
* rename index
* MM-23807: Refactor web_conn
- Unexport some struct fields and constants which are not necessary
to be accessed from outside the package. This will help us moving
the entire websocket handling code to a separate package later.
- Change some empty string checks to check for empty string rather
than doing a len check which is more idiomatic. Both of them compile
to the same code. So it doesn't make a difference performance-wise.
- Remove redundant ToJson calls to get the length.
- Incorporate review comments
- Unexport some more methods
* Fix field name
* Run make app-layers
* Add note on hub check
* MM-23800: remove goroutineID and stack printing
Each hub has a goroutineID which is calculated with a known hack.
The FAQ clearly explains why goroutines don't have an id:
https://golang.org/doc/faq#no_goroutine_id.
We only added that because sometimes the hub would be deadlocked and
having the goroutineID would be useful when getting the stack trace.
This is also problematic in stress tests because the hubs would
frequently get overloaded and the logs would unnecessarily have stack traces.
But that was in the past, and we have done extensive testing with
load tests and fuzz testing to smooth any rough edges remaining.
Including adding additional metrics for hub buffer size.
Monitoring the metrics is a better way to approach this problem.
Therefore, we remove these kludges from the code.
* Also remove deadlock checking code
There is no need for that anymore since
we are getting rid of the stack printing anyways.
Let's do a wholesale refactor and clean up the codebase.
Co-authored-by: mattermod <mattermod@users.noreply.github.com>
* MM-22044: Fix panic on web_conn send hello
(*Hub).Start is the central place for sending all web connection
related traffic. However, there was this one call to (*WebConn).Hello
which tried to send a message to a webconn separately.
This was a rare case, but it did occur under stress conditions generated
from a load test.
When the websocket send SEND_QUEUE_SIZE would get filled up and we would
attempt to make a broadcast, the non-blocking send would close the Send
channel of the web connection. During that time, if a web connection
would try to perform a broadcast, it would try to send to a closed channel
and cause a panic.
The solution is to bring back the sending of hello into the same goroutine
inside (*Hub).Start so that all state is centralised and we avoid
sending to a closed channel by sending the hello message inside the registering
code itself.
* Adding non-blocking send
* Simplify things
* Remove test
* Bring sendHello back
* Improve code further
Co-authored-by: mattermod <mattermod@users.noreply.github.com>
Hubs property has been detected by the race condition detector
as a possible one because we could set the Hubs variable
and also we're spawning goroutines that could ask for that property
This condition is not happening right now because we're setting the
Hubs variable in the server start and after that, we're spawning the
goroutines but the tests are failing because of this and in the future
this could be a problem
While invalidating a channel, we send out a cluster message
from the app layer as well as from the localcachelayer.
This causes multiple redundant messages to be sent out.
And additionally multiple cluster handlers were being registered
from the app layer as well as the localcachelayer.
We fix this by removing the cluster broadcast from the app layer
and directly calling the store method. We also remove
the duplicate event handler registration.
* Make WebSocketEvent type immutable
* Update code to use updated immutable WebSocketEvent type
* Export WebSocketEvent fields and mark them as deprecated
* Consistent license message for all the go files
* Fixing the last set of unconsistencies with the license headers
* Addressing PR review comments
* Fixing busy.go and busy_test.go license header
