When a user disconnects from the hub, we would spawn off a goroutine
which would make a cluster request, and then update the user status
as offline in the DB.
This was another case of unbounded concurrency where the number of
goroutines spawned was user controlled. Therefore, we would see
a clear spike in DB connections on master when a lot of users
would suddenly disconnect.
To fix this, we implement concurrency control in two areas:
1. In making the cluster request. We implement a counting semaphore
per-hub to avoid making unbounded cluster requests.
2. We use a buffered channel with a periodic flusher to process
status updates.
We also add a new store method to upsert multiple statuses
in a single query. The statusUpdateThreshold is set to 32, which means
no more than 32 rows will be upserted at one time, keeping the
SQL query load reasonable.
https://mattermost.atlassian.net/browse/MM-64298
```release-note
We improve DB connection spikes on user disconnect
by processing status updates in batches.
```
We switch to using iterators introduced in Go 1.23
to make iteration alloc-free and fast. And since
element removal is allowed while iterating a map,
this also means we don't need to even copy the slice
any more.
While here, we also address the comment https://github.com/mattermost/mattermost/pull/30178#discussion_r1954862151.
I have simply gone back to using []string as the map
entry rather than a type alias or a redirection with
a struct.
https://mattermost.atlassian.net/browse/MM-63130
```release-note
NONE
```
* Changed back nil to len
```release-note
NONE
```
* fixing unused assignment
```release-note
NONE
```
* add benchmark
```
goos: linux
goarch: amd64
pkg: github.com/mattermost/mattermost/server/v8/channels/app/platform
cpu: Intel(R) Core(TM) i5-8265U CPU @ 1.60GHz
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
HubConnIndexIterator/2_users-8 93.53n ± 1% 38.09n ± 1% -59.27% (p=0.000 n=10)
HubConnIndexIterator/3_users-8 106.30n ± 0% 38.41n ± 1% -63.86% (p=0.000 n=10)
HubConnIndexIterator/4_users-8 111.30n ± 1% 38.66n ± 1% -65.27% (p=0.000 n=10)
geomean 103.4n 38.39n -62.89%
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
HubConnIndexIterator/2_users-8 16.00 ± 0% 24.00 ± 0% +50.00% (p=0.000 n=10)
HubConnIndexIterator/3_users-8 24.00 ± 0% 24.00 ± 0% ~ (p=1.000 n=10) ¹
HubConnIndexIterator/4_users-8 32.00 ± 0% 24.00 ± 0% -25.00% (p=0.000 n=10)
geomean 23.08 24.00 +4.00%
¹ all samples are equal
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
HubConnIndexIterator/2_users-8 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹
HubConnIndexIterator/3_users-8 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹
HubConnIndexIterator/4_users-8 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹
geomean 1.000 1.000 +0.00%
¹ all samples are equal
```
```release-note
NONE
```
* ForChannel test as well
```release-note
NONE
```
* review comments
```release-note
NONE
```
* fix lint errors
```release-note
NONE
```
---------
Co-authored-by: Mattermost Build <build@mattermost.com>
If the deadQueue was full, we would be incorrectly sending
the wrong index for dqPtr. Fixed that and added a test case.
https://mattermost.atlassian.net/browse/MM-63304
```release-note
NONE
```
When Channel iteration mode is enabled, we need to ensure that
channel scoped events do not fall through to the connIndex.All()
condition. This is possible because there are multiple hubs
in a given system. So a single event will flow through all of them
and in some hubs, a channel scoped event might not have any connections.
In that case, we need to stop processing further.
https://mattermost.atlassian.net/browse/MM-62960
```release-note
NONE
```
When we added iteration by channelID, this was a known tradeoff during that.
However, it has been observed that the regular connection removal function
creates considerable blocking of the processing loop, leading to high
CPU usage and API latencies.
To fix this, we add a reverse mapping of channelIDs to connections
and their positions in the slice. This helps us to remove the connection
from the slice without iteration.
Unfortunately, this still needs to iterate through all channelIDs
during invalidation of the channel member cache. However, the user
cache invalidation is not a regular activity. So it should be an acceptable
tradeoff to make.
https://mattermost.atlassian.net/browse/MM-62960
```release-note
A new config knob ServiceSettings.EnableWebHubChannelIteration which allows a user to control the performance of websocket broadcasting. By default, this setting is turned off. If it is turned on, it improves the websocket broadcasting performance at the expense of poor performance when users join/leave a channel. It is not recommended to turn it on unless you have atleast 200,000 concurrent users actively using MM.
```
Co-authored-by: Mattermost Build <build@mattermost.com>
We do a cluster request to get the active and dead queues
from other nodes in the cluster to sync any missing
information.
We check the dead queue in the other nodes to see
if there's been any message loss or not. Accordingly,
we send just the active queue or both active and dead queues.
There's still an edge case that is left out where
a client could have potentially connected and reconnected
to multiple nodes leaving multiple active queues
in multiple nodes. We don't handle this scenario
because then potentially we need to create
a slice of sendQueueSize * number_of_nodes. And then
this can happen again, leading to an infinite increase
in sendQueueSize.
We leave this edge-case to Redis, acknowledging
a limitation in our architecture.
In this PR, when there's no message loss, we just
take the active queue from the last node it connected
to.
And if there's message loss where the client's
seqNum is within the last node's dead queue, we also
handle that.
But if there's severe message loss where the client's
seqNum falls within the dead queue of another node, then
we just send the data from that node to reconstruct the
data as much as possible. It could be possible to set
a new connection ID in this case, but this involves
more data transfer always from all nodes and recomputing
the state in the requestor node.
https://mattermost.atlassian.net/browse/MM-61904
```release-note
NONE
```
Co-authored-by: Mattermost Build <build@mattermost.com>
Tests at very high scale indicates that the iteration
of all connections during websocket broadcast starts
to become a bottleneck.
To optimize this, we move the channelMember cache from
inside web_conn.go to the hubConnectionIndex.
This involves adding a new map keyed by the channelID
and containing all webConns where the user is a member
of that channel. Subsequently, a new method needed to
be added to invalidate the cache which previously
used to happen in web_conn.
And as a last step, we remove the cache from web_conn
to reduce SQL queries to the DB.
https://mattermost.atlassian.net/browse/MM-61130
```release-note
NONE
```
This originated from https://github.com/mattermost/mattermost/issues/15249.
However, the original idea was discarded https://github.com/mattermost/mattermost/issues/15249#issuecomment-709713065
as being too complicated to implement. Then I had another
idea to implement it just for session objects.
My thinking was that since every single request allocates a new
session struct, it would be good to use a sync.Pool for that.
However, 4 years later, now we know that the primary bottleneck
in app performance comes from websocket event marshalling.
Therefore, while it would be good to do this, it is difficult
to do it correctly (as shown by the numerous racy tests).
Hence, reverting this.
```release-note
NONE
```
PR: https://github.com/mattermost/mattermost/pull/27204 fixed the issue
where `invalidateWebConnSessionCacheForUser` was not broadcasted across
the cluster. However, we became too conservative and started to invalidate
the session cache as well.
But the session cache only contains the session details for the user
and the team membership. There is no channel membership related info.
Therefore, we avoid that and improve performance as well.
https://mattermost.atlassian.net/browse/MM-60478
```release-note
NONE
```
Co-authored-by: Mattermost Build <build@mattermost.com>
We were invalidating the userProfileByIds cache unnecessarily
when it wasn't really needed. It's just that the InvalidateCacheForUser
method wiped off all user related caches, when we just needed to wipe
user-channel related caches.
To fix this, we split the user cache invalidation method into two
and just call the InvalidateChannelCacheForUser method from app/channel.go.
https://mattermost.atlassian.net/browse/MM-59827
```release-note
NONE
```
* [MM-58355] Send invalidate cache message across the cluster so that websocket connections on other instances are invalidated correctly
* Add suggestion to clear the session cache on the local node as well
* Force read from master DB when gettting channel members for websocket to avoid any DB sync issues
* PR feedback
* Missed generated files
We were setting the user status to offline without
checking for connections on other nodes in a cluster.
Now we implement a request-response mechanism for the whole
cluster and we check that before setting a user to offline.
https://mattermost.atlassian.net/browse/MM-57153
```release-note
Fix a bug where the user status would incorrectly be set to offline
without checking for connections in other nodes in an HA cluster.
```
Co-authored-by: Ibrahim Serdar Acikgoz <serdaracikgoz86@gmail.com>
Co-authored-by: Mattermost Build <build@mattermost.com>
Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>
We randomly used double the number of hubs. A proper test indicates
that using the same number of hubs as CPUs gives better performance.
https://mattermost.atlassian.net/browse/MM-56355
```release-note
NONE
```
PR https://github.com/mattermost/mattermost/pull/22560
introduced a regression in the case where we had
multiple connections for a single user.
Because if connIndex.Remove was called during the iteration
from connIndex.ForUser, then the slice would be modified
during iteration and if a connection got removed,
then a good connection would move from the last index
to the current index. But since we would be actively
iterating, the last index would be read as nil and we would
never be able to reach the good connection.
We fix this by returning a copy of the original slice
if there are more than one elements in the slice.
https://mattermost.atlassian.net/browse/MM-56260
```release-note
Fix a bug where if there were multiple websocket
connections from a single user, then in case one connection
got removed during a broadcast, there was a possibility
that the other good connection might not get the event.
```
* MM-54238 Initial implementation
* MM-54238 Move websocket hook into app package
* MM-54238 Add tests for mentions in posted websocket messages
* Fix styling
* Fix other styling
* Idiomatic ID naming for new code
* Fix more styles
* Separate hooks to add mentions and followers
* Improved error handling for invalid types in hooks
* Rename HasChanges to ShouldProcess
* Pass broadcast hooks through hubStart
* Add test helper for asserting json unmarshaling
* Fix missing arguments in tests
* Ensure broadcast hooks are sent across the cluster and not to users
* Ensure tests actually cover following a post
* Fix code broken by merge
* Go vet again...
* Deep copy event before processing it with hooks
* Replace RemoveBroadcastHooks with WithoutBroadcastHooks
* Address feedback
* Add helper to fix type information for hook args
* Wrap WebSocketEvent and simplify BroadcastHook
* Address feedback
* Address feedback
After reliable websockets were introduced,
we would keep pushing to a webConn even after
the client has disconnected. This would lead
to the websocket.slow/full and eventually
the disconnect messages which unnecessarily
confuse the admin.
We don't log them if the connection is inactive.
We use the active field and convert it to an atomic
to use more widely.
```release-note
NONE
```
https://mattermost.atlassian.net/browse/MM-52532
- Replace golint with revive
- Add makezero linter
- Fix all the required linter failures
Some issues in enterprise and public modules
are yet to be fixed. We send this to expediate things.
It was a good decision in hindsight to keep the public module as 0.x
because this would have been a breaking change again.
https://mattermost.atlassian.net/browse/MM-53032
```release-note
Changed the Go module path from github.com/mattermost/mattermost-server/server/v8 to github.com/mattermost/mattermost/server/v8.
For the public facing module, it's path is also changed from github.com/mattermost/mattermost-server/server/public to github.com/mattermost/mattermost/server/public
```
* platform/web_conn: fix a possible data race issue
* reflect review comments
---------
Co-authored-by: Ibrahim Serdar Acikgoz <ibrahim@ibrahims-mac.local>
Co-authored-by: Mattermost Build <build@mattermost.com>
