* 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
* MM-14138: Adding new "VIEW_MEMBERS" permissions restrict the scope of users visibility
* Fixing gofmt
* Fixing broken tests
* Addressing PR review comments from Miguel de la Cruz
* Removed hack
* A bit nicer and cleaner code in the UserBelongsToChannels function
* Adding cluster cache invalidation for user team ids
* Checking in the correct order permissions to not leek existency information
* Adding restrictions to TeamMembers and User status requests
* Fixing tests
* Fixing status endpoint permissions checks
* Adding more tests
* Fixing tests
* More tests and making the restrictions query based only on joins
* Adding more tests
* Adding more tests
* fixing merge problems
* Reverting status changes to avoid performance issues
* Adding more tests
* Fixing test
* i18n extract
* Adding extra method for get restrictions for a team
* Add the new elasticsearch functions to search users with restrictions
* Add missing translation string
* Rename restrictedChannelIds to restrictedToChannels
* Remove ToDo
* Adding the permission to the SystemAdmin role during permissions migrations
The websocket hub, on stopping, closes all associated websocket connections, and each connection
in turn tries to unregister itself with the hub, but this naturally creates a deadlock.
This PR reverts the line commented (incorretly...) at
https://github.com/mattermost/mattermost-server/pull/9863/files#r235583929.
* fix webconn close semantics
Avoid race conditions in WebConn on shutdown by closing channels to guarantee all readers are notified. Wrap this with sync.Once to avoid closing the channel more than once.
* web_hub_test.go
* webhub: fix race condition on shutdown
Ensure that if the webhub shuts down in the process of sending, the caller unblocks given that the webhub will no longer consume incoming events.
* panic if app shutdown takes >30 seconds
* simplify WebConn::Pump channel semantics too
* Moving goroutine pool
* Auto refactor
* Moving plugins.
* Auto refactor
* Moving fields to server
* Auto refactor
* Removing siteurl duplication.
* Moving reset of app fields
* Auto refactor
* Formatting
* Moving niling of Server to after last use
* Fixing unit tests.
* Implementing structured logging
* Changes to en.json to allow refactor to run.
* Fixing global logger
* Structured logger initalization.
* Add caller.
* Do some log redirection.
* Auto refactor
* Cleaning up l4g reference and removing dependancy.
* Removing junk.
* Copyright headers.
* Fixing tests
* Revert "Changes to en.json to allow refactor to run."
This reverts commit fd8249e99bcad0231e6ea65cd77c32aae9a54026.
* Fixing some auto refactor strangeness and typo.
* Making keys more human readable.
* Adding Reaction store cache layer example
* Implementing reaction store in new caching system.
* Redis for reaction store
* Adding redis library
* Adding invalidation for DeleteAllWithEmojiName and other minor enhancements
