Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encode groups and conversations sequentially to fix iOS pool errors #462

Merged
merged 4 commits into from
Aug 5, 2024
Merged

Encode groups and conversations sequentially to fix iOS pool errors #462

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c597e9
fix: encode groups and conversations sequentially instead of as many …
cameronvoell Aug 5, 2024
913a6a0
removing created at tests from UI since order is not reliable
cameronvoell Aug 5, 2024
b7f91b7
add test showing large listGroups without ios pool error
cameronvoell Aug 5, 2024
be21834
lint fix
cameronvoell Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions example/src/TestScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { useRoute } from '@react-navigation/native'
import React, { useEffect, useState } from 'react'
import { View, Text, Button, ScrollView } from 'react-native'

import { createdAtTests } from './tests/createdAtTests'
import { groupPermissionsTests } from './tests/groupPermissionsTests'
import { groupTests } from './tests/groupTests'
import { restartStreamTests } from './tests/restartStreamsTests'
Expand Down Expand Up @@ -107,7 +106,6 @@ export enum TestCategory {
all = 'all',
tests = 'tests',
group = 'group',
createdAt = 'createdAt',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected to be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, these tests were initially just documenting different behaviors between iOS and Android, but looking at them now, they just pass / fail randomly based off random order that groups are returned. tldr, dont think these tests are useful at the moment, and are just creating extra noise, in my opinion, as the person who wrote them :-P

restartStreans = 'restartStreams',
groupPermissions = 'groupPermissions',
}
Expand All @@ -121,7 +119,6 @@ export default function TestScreen(): JSX.Element {
const allTests = [
...tests,
...groupTests,
...createdAtTests,
...restartStreamTests,
...groupPermissionsTests,
]
Expand Down
96 changes: 44 additions & 52 deletions example/src/tests/createdAtTests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,9 @@
await alix.conversations.syncGroups()
const alixGroups = await alix.conversations.listGroups()

// BUG - List returns in Reverse Chronological order on iOS
// and Chronological order on Android
const first = isIos() ? 1 : 0
const second = isIos() ? 0 : 1

Check warning on line 33 in example/src/tests/createdAtTests.ts

View workflow job for this annotation

GitHub Actions / lint 

Delete `⏎`
const first = 0
const second = 1
assert(alixGroups.length === 2, 'Alix should have two groups')
assert(
alixGroups[first].id === alixGroup.id,
Expand All @@ -47,14 +46,15 @@
alixGroups[second].id === boGroup.id,
'Bo group createdAt should match'
)

// Below assertion fails on Android
if (isIos()) {
assert(
alixGroups[second].createdAt === boGroup.createdAt,
'Second group returned from listGroups shows ' +
alixGroups[second].createdAt +
' but should be ' +
boGroup.createdAt
alixGroups[second].createdAt +

Check warning on line 55 in example/src/tests/createdAtTests.ts

View workflow job for this annotation

GitHub Actions / lint 

Delete `··`
' but should be ' +

Check warning on line 56 in example/src/tests/createdAtTests.ts

View workflow job for this annotation

GitHub Actions / lint 

Delete `··`
boGroup.createdAt

Check warning on line 57 in example/src/tests/createdAtTests.ts

View workflow job for this annotation

GitHub Actions / lint 

Delete `··`
)
}
return true
Expand All @@ -80,35 +80,37 @@
assert(alixGroups.length === 2, 'alix should have two groups')

// Returns reverse Chronological order on Android and iOS
const first = 1
const second = 0
const first = 0
const second = 1
assert(
alixGroups[1].topic === alixGroup.topic,
alixGroups[first].topic === alixGroup.topic,
'First group returned from listGroups shows ' +
alixGroups[1].topic +
alixGroups[first].topic +
' but should be ' +
alixGroup.topic
)
assert(
alixGroups[0].topic === boGroup.topic,
alixGroups[second].topic === boGroup.topic,
'Second group returned from listGroups shows ' +
alixGroups[0].topic +
alixGroups[second].topic +
' but should be ' +
boGroup.topic
)
assert(
alixGroups[first].createdAt === alixGroup.createdAt,
'alix group returned from listGroups shows createdAt ' +
alixGroups[1].createdAt +
' but should be ' +
alixGroup.createdAt
)
// Below assertion fail on Android

// Below assertion fails on Android
if (isIos()) {
assert(
alixGroups[first].createdAt === alixGroup.createdAt,
'alix group returned from listGroups shows createdAt ' +
alixGroups[first].createdAt +
' but should be ' +
alixGroup.createdAt
)

assert(
alixGroups[second].createdAt === boGroup.createdAt,
'bo group returned from listGroups shows createdAt ' +
alixGroups[0].createdAt +
alixGroups[second].createdAt +
' but should be ' +
boGroup.createdAt
)
Expand Down Expand Up @@ -152,19 +154,15 @@
'second ' + allGroups[1].id + ' != ' + caroGroup.id
)

// CreatedAt returned from stream matches createAt from create function
// Assertion below fails on Android
if (isIos()) {
assert(
allGroups[0].createdAt === boGroup.createdAt,
'first ' + allGroups[0].createdAt + ' != ' + boGroup.createdAt
)
assert(
allGroups[0].createdAt === boGroup.createdAt,
'first ' + allGroups[0].createdAt + ' != ' + boGroup.createdAt
)

assert(
allGroups[1].createdAt === caroGroup.createdAt,
'second ' + allGroups[1].createdAt + ' != ' + caroGroup.createdAt
)
}
assert(
allGroups[1].createdAt === caroGroup.createdAt,
'second ' + allGroups[1].createdAt + ' != ' + caroGroup.createdAt
)

cancelStream()

Expand Down Expand Up @@ -207,18 +205,14 @@
'second ' + allGroups[1].topic + ' != ' + caroGroup.topic
)

// CreatedAt returned from stream matches createAt from create function
// Assertion below fails on Android
if (isIos()) {
assert(
allGroups[0].createdAt === boGroup.createdAt,
'first ' + allGroups[0].createdAt + ' != ' + boGroup.createdAt
)
assert(
allGroups[1].createdAt === caroGroup.createdAt,
'second ' + allGroups[1].createdAt + ' != ' + caroGroup.createdAt
)
}
assert(
allGroups[0].createdAt === boGroup.createdAt,
'first ' + allGroups[0].createdAt + ' != ' + boGroup.createdAt
)
assert(
allGroups[1].createdAt === caroGroup.createdAt,
'second ' + allGroups[1].createdAt + ' != ' + caroGroup.createdAt
)

cancelStream()

Expand All @@ -243,8 +237,8 @@

// BUG - List returns in Chronological order on iOS
// and reverse Chronological order on Android
const first = isIos() ? 0 : 1
const second = isIos() ? 1 : 0
const first = 0
const second = 1

assert(
alixConversations[first].topic === alixConversation.topic,
Expand Down Expand Up @@ -284,10 +278,8 @@
const alixConversations = await alix.conversations.listAll()
assert(alixConversations.length === 2, 'alix should have two conversations')

// BUG - List returns in Chronological order on iOS
// and reverse Chronological order on Android
const first = isIos() ? 0 : 1
const second = isIos() ? 1 : 0
const first = 0
const second = 1

// List returns in reverse Chronological order
assert(
Expand Down
54 changes: 52 additions & 2 deletions example/src/tests/groupTests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
assert,
createClients,
delayToPropogate,
isIos,

Check warning on line 9 in example/src/tests/groupTests.ts

View workflow job for this annotation

GitHub Actions / lint 

'isIos' is defined but never used
} from './test-utils'
import {
Client,
Expand Down Expand Up @@ -49,6 +49,28 @@
return true
})

async function createGroups(
client: Client,
peers: Client[],
numGroups: number,
numMessages: number
): Promise<Group[]> {
const groups = []
const addresses: string[] = peers.map((client) => client.address)
for (let i = 0; i < numGroups; i++) {
const group = await client.conversations.newGroup(addresses, {
name: `group ${i}`,
imageUrlSquare: `www.group${i}.com`,
description: `group ${i}`,
})
groups.push(group)
for (let i = 0; i < numMessages; i++) {
await group.send({ text: `Message ${i}` })
}
}
return groups
}

test('calls preAuthenticateToInboxCallback when supplied', async () => {
let isCallbackCalled = 0
let isPreAuthCalled = false
Expand Down Expand Up @@ -184,6 +206,34 @@
return true
})

test('testing large group listing with metadata performance', async () => {
const [alixClient, boClient] = await createClients(2)

Check warning on line 210 in example/src/tests/groupTests.ts

View workflow job for this annotation

GitHub Actions / lint 

Delete `··`

Check warning on line 211 in example/src/tests/groupTests.ts

View workflow job for this annotation

GitHub Actions / lint 

Delete `··`
await createGroups(alixClient, [boClient], 50, 10)

Check warning on line 212 in example/src/tests/groupTests.ts

View workflow job for this annotation

GitHub Actions / lint 

Delete `··`

Check warning on line 213 in example/src/tests/groupTests.ts

View workflow job for this annotation

GitHub Actions / lint 

Delete `··`
let start = Date.now()

Check warning on line 214 in example/src/tests/groupTests.ts

View workflow job for this annotation

GitHub Actions / lint 

Delete `··`
let groups = await alixClient.conversations.listGroups()
let end = Date.now()
console.log(`Alix loaded ${groups.length} groups in ${end - start}ms`)

start = Date.now()
await alixClient.conversations.syncGroups()
end = Date.now()
console.log(`Alix synced ${groups.length} groups in ${end - start}ms`)

start = Date.now()
await boClient.conversations.syncGroups()
end = Date.now()
console.log(`Bo synced ${groups.length} groups in ${end - start}ms`)

start = Date.now()
groups = await boClient.conversations.listGroups()
end = Date.now()
console.log(`Bo loaded ${groups.length} groups in ${end - start}ms`)

return true
})

test('can drop a local database', async () => {
const [client, anotherClient] = await createClients(2)

Expand Down Expand Up @@ -1027,8 +1077,8 @@
// Verify information in listed containers is correct
// BUG - List All returns in Chronological order on iOS
// and reverse Chronological order on Android
const first = isIos() ? 1 : 0
const second = isIos() ? 0 : 1
const first = 0//isIos() ? 1 : 0
const second = 1//isIos() ? 0 : 1
if (
listedContainers[first].topic !== boGroup.topic ||
listedContainers[first].version !== ConversationVersion.GROUP ||
Expand Down
66 changes: 23 additions & 43 deletions ios/XMTPModule.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,44 +400,31 @@ public class XMTPModule: Module {
}

let conversations = try await client.conversations.list()

return try await withThrowingTaskGroup(of: String.self) { group in
for conversation in conversations {
group.addTask {
await self.conversationsManager.set(conversation.cacheKey(inboxId), conversation)
return try ConversationWrapper.encode(conversation, client: client)
}
}

var results: [String] = []
for try await result in group {
results.append(result)
}

return results

var results: [String] = []
Copy link
Contributor Author

@cameronvoell cameronvoell Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went ahead and removed TaskGroup from listAll and listConversations since it could lead to similar issues with too many db connections, and I did not observe any slowdown in the test for loading 2000 conversations.

before the change I saw "LOG Loaded 1995 conversations in 5717ms" and after I saw actually slightly faster "LOG Loaded 1995 conversations in 5595ms"

for conversation in conversations {
await self.conversationsManager.set(conversation.cacheKey(inboxId), conversation)
let encodedConversation = try ConversationWrapper.encode(conversation, client: client)
results.append(encodedConversation)
}

return results
}

AsyncFunction("listGroups") { (inboxId: String) -> [String] in
guard let client = await clientsManager.getClient(key: inboxId) else {
throw Error.noClient
}
let groupList = try await client.conversations.groups()
return try await withThrowingTaskGroup(of: String.self) { taskGroup in
for group in groupList {
taskGroup.addTask {
await self.groupsManager.set(group.cacheKey(inboxId), group)
return try GroupWrapper.encode(group, client: client)
}
}

var results: [String] = []
for try await result in taskGroup {
results.append(result)
}

return results

var results: [String] = []
for group in groupList {
await self.groupsManager.set(group.cacheKey(inboxId), group)
let encodedGroup = try GroupWrapper.encode(group, client: client)
results.append(encodedGroup)
}

return results
}

AsyncFunction("listAll") { (inboxId: String) -> [String] in
Expand All @@ -446,21 +433,14 @@ public class XMTPModule: Module {
}
let conversationContainerList = try await client.conversations.list(includeGroups: true)

return try await withThrowingTaskGroup(of: String.self) { taskGroup in
for conversation in conversationContainerList {
taskGroup.addTask {
await self.conversationsManager.set(conversation.cacheKey(inboxId), conversation)
return try ConversationContainerWrapper.encode(conversation, client: client)
}
}

var results: [String] = []
for try await result in taskGroup {
results.append(result)
}

return results
var results: [String] = []
for conversation in conversationContainerList {
await self.conversationsManager.set(conversation.cacheKey(inboxId), conversation)
let encodedConversationContainer = try ConversationContainerWrapper.encode(conversation, client: client)
results.append(encodedConversationContainer)
}

return results
}

AsyncFunction("loadMessages") { (inboxId: String, topic: String, limit: Int?, before: Double?, after: Double?, direction: String?) -> [String] in
Expand Down
Loading