Skip to content

Commit

Permalink
bugfix: fix a random crash in FFI code.
Browse files Browse the repository at this point in the history
The crash would confusingly happen in other tests, sometimes
even JS tests! This was because I forgot to clean up some FFI
objects. Go's GC would eventually deallocate the matching Go object
which then automatically calls `.Destroy()` on the FFI object.

However, if this ran sufficiently late then the supporting Tokio
runtime may no longer exist, meaning rust code cannot use async/await.
This would cause a panic with:

> thread '<unnamed>' panicked at 'there is no reactor running, must be called from the context of a Tokio 1.x runtime'
  • Loading branch information
kegsay committed Jan 29, 2024
1 parent de285ea commit 5d38d2d
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 3 deletions.
11 changes: 10 additions & 1 deletion internal/api/rust/rust.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,16 @@ func (c *RustClient) MustStartSyncing(t ct.TestLike) (stopSyncing func()) {
// Tests should call stopSyncing() at the end of the test.
func (c *RustClient) StartSyncing(t ct.TestLike) (stopSyncing func(), err error) {
t.Helper()
syncService, err := c.FFIClient.SyncService().Finish()
// It's critical that we destroy the sync_service_builder object before we return.
// You might be tempted to chain this function call e.g FFIClient.SyncService().Finish()
// but if you do that, the builder is never destroyed. If that happens, the builder will
// eventually be destroyed by Go finialisers running, but at that point we may not have
// a tokio runtime running anymore. This will then cause a panic with something to the effect of:
// > thread '<unnamed>' panicked at 'there is no reactor running, must be called from the context of a Tokio 1.x runtime'
// where the stack trace doesn't hit any test code, but does start at a `free_` function.
sb := c.FFIClient.SyncService()
defer sb.Destroy()
syncService, err := sb.Finish()
if err != nil {
return nil, fmt.Errorf("[%s]failed to make sync service: %s", c.userID, err)
}
Expand Down
7 changes: 7 additions & 0 deletions internal/deploy/callback_addon.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import (
"github.com/matrix-org/complement/must"
)

var lastTestName string

type CallbackData struct {
Method string `json:"method"`
URL string `json:"url"`
Expand All @@ -23,6 +25,10 @@ type CallbackData struct {
// Returns the URL of the callback server for use with WithMITMOptions, along with a close function
// which should be called when the test finishes to shut down the HTTP server.
func NewCallbackServer(t *testing.T, cb func(CallbackData)) (callbackURL string, close func()) {
if lastTestName != "" {
t.Logf("WARNING: NewCallbackServer called without closing the last one. Check test '%s'", lastTestName)
}
lastTestName = t.Name()
mux := http.NewServeMux()
mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
var data CallbackData
Expand All @@ -46,5 +52,6 @@ func NewCallbackServer(t *testing.T, cb func(CallbackData)) (callbackURL string,
go srv.Serve(ln)
return fmt.Sprintf("http://host.docker.internal:%d", port), func() {
srv.Close()
lastTestName = ""
}
}
4 changes: 2 additions & 2 deletions tests/room_keys_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,14 +240,14 @@ func testRoomKeyIsNotCycledOnClientRestartJS(t *testing.T, clientType api.Client
alice = tc.MustCreateClient(t, tc.Alice, clientType, WithPersistentStorage())
defer alice.Close(t)
alice.Login(t, alice.Opts()) // login should work
alice.StartSyncing(t)
alice2StopSyncing, _ := alice.StartSyncing(t)
defer alice2StopSyncing()

// now send another message from Alice, who should NOT send another new room key
wantMsgBody = "Another Test Message"
waiter = bob.WaitUntilEventInRoom(t, roomID, api.CheckEventHasBody(wantMsgBody))
alice.SendMessage(t, roomID, wantMsgBody)
waiter.Wait(t, 5*time.Second)

})

// we should have seen a /sendToDevice call by now. If we didn't, this implies we didn't cycle
Expand Down

0 comments on commit 5d38d2d

Please sign in to comment.