slacktest GetSeenOutboundMessages is updated asynchronously and can cause test failures due to race conditions #1361
Comments
askreet
added a commit
to askreet/slack
that referenced
this issue
Dec 13, 2024
This patch addresses issue slack-go#1361. The storage backing GetSeenOutboundMessages is updated in a goroutine that may or may not be executed in time for assertions against this method. This patch updates the storage to be updated synchronously in the handler, while maintaining the asynchronous queue behavior for websockets handlers. The test case has been updated to remove the sleep statement that likely worked around this very issue.
askreet
added a commit
to askreet/slack
that referenced
this issue
Dec 13, 2024
This patch addresses issue slack-go#1361. The storage backing GetSeenOutboundMessages is updated in a goroutine that may or may not be executed in time for assertions against this method. This patch updates the storage to be updated synchronously in the handler, while maintaining the asynchronous queue behavior for websockets handlers. The test case has been updated to remove the sleep statement that likely worked around this very issue. I opted to change the locking behavior to be more closely related with the messageCollection type. There is a smaller version of this fix that move the lock/update/unlock block into the callsite of each queue update, if that is preferable.
nlopes
pushed a commit
to askreet/slack
that referenced
this issue
Feb 6, 2025
This patch addresses issue slack-go#1361. The storage backing GetSeenOutboundMessages is updated in a goroutine that may or may not be executed in time for assertions against this method. This patch updates the storage to be updated synchronously in the handler, while maintaining the asynchronous queue behavior for websockets handlers. The test case has been updated to remove the sleep statement that likely worked around this very issue. I opted to change the locking behavior to be more closely related with the messageCollection type. There is a smaller version of this fix that move the lock/update/unlock block into the callsite of each queue update, if that is preferable.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What happened
If a test case is using the slacktest server and quickly tests a function that sends a message followed by a call to GetSeenOutboundMessages it is possible that the goroutine which updates that storage has not yet been scheduled.
I have a branch that addresses this by separating the websocket queueing (done in a goroutine) from updating the "seen messages" storage. I'll post a PR shortly.
Expected behavior
GetSeenOutboundMessages should reliably show messages for which the server has responded with a successful HTTP response.
Steps to reproduce
Remove the sleep that works around this issue in slacktest.Server's own test code here, and the test fails most of the time.
reproducible code
manifest.yaml
Versions
The text was updated successfully, but these errors were encountered: