fix: Reorder the closing of services, prevent sagas running multiple times and close backend server properly #2499

leblowl · 2024-05-01T18:25:31Z

Fixes:

We were closing LevelDB before closing OrbitDB which resulted in an exception.

Solution for Clear LevelDB after OrbitDB when leaving a community #2497
Sagas were running multiple times after leaving a community due to our use of takeEvery.

Solution for Sagas run multiple times after leaving and rejoining community #2509
The backend server wasn't getting closed properly when the app went to background on iOS.

Solution for Stuck on 'Connecting process started' due to missing community on iOS #2500
There was a bug in the (old) version of OrbitDB that we are using that caused it to send heads in a loop.

Solution for App can get stuck on 'Loading messages' when joining #2496 and Stuck on splash screen when leaving community #2512

Pull Request Checklist

I have linked this PR to a related GitHub issue.
I have added a description of the change (and Github issue number, if any) to the root CHANGELOG.md.

(Optional) Mobile checklist

Please ensure you completed the following checks if you did any changes to the mobile package:

I have run e2e tests for mobile
I have updated base screenshots for visual regression tests

We were closing LevelDB before closing OrbitDB which resulted in an exception.

leblowl · 2024-05-07T16:48:07Z

packages/backend/src/backendManager.ts

    const connectionsManager = app.get<ConnectionsManagerService>(ConnectionsManagerService)
-    await connectionsManager.pause()
+    connectionsManager.pause()


No need for async here since nothing awaits the event handler

leblowl · 2024-05-07T16:50:11Z

packages/backend/src/nest/connections-manager/connections-manager.service.ts

-    if (this.libp2pService) {
-      this.logger('Stopping libp2p')
-      await this.libp2pService.close()
-    }
  }


Re-organizing. Data server should get closed first and then OrbitDB, then LibP2P

leblowl · 2024-05-07T16:50:59Z

packages/backend/src/nest/connections-manager/connections-manager.service.ts

  }

  public async pause() {
    this.logger('Pausing!')
-    this.logger('Closing socket!')
-    this.closeSocket()
+    await this.closeSocket()


I remove the log because there is a similar log in sockerService.close, but we can keep it too, just let me know.

leblowl · 2024-05-07T16:54:12Z

packages/backend/src/nest/connections-manager/connections-manager.service.ts

    this.logger('Pausing libp2pService!')
    this.peerInfo = await this.libp2pService?.pause()
    this.logger('Found the following peer info on pause: ', this.peerInfo)
  }

  public async resume() {
    this.logger('Resuming!')
-    this.logger('Reopening socket!')
    await this.openSocket()


Again, I remove the log because there is a similar log in socketService.listen. We can keep both also, just let me know.

leblowl · 2024-05-07T17:02:18Z

packages/mobile/src/store/init/startConnection/startConnection.saga.ts

@@ -49,17 +61,20 @@ export function* startConnectionSaga(
  })
  yield* fork(handleSocketLifecycleActions, socket, action.payload)
  // Handle opening/restoring connection
-  yield* takeEvery(initActions.setWebsocketConnected, setConnectedSaga, socket)
+  yield* takeLeading(initActions.setWebsocketConnected, setConnectedSaga, socket)


This only needs to happen in sequence

leblowl · 2024-05-07T17:02:56Z

packages/mobile/src/store/init/startConnection/startConnection.saga.ts

+
+  // Handle suspending current connection
+  const suspendAction = yield* take(initActions.suspendWebsocketConnection)
+  yield* call(cancelRootTaskSaga, task, suspendAction)


This only needs to happen once (takeEvery is not necessary and causes issues)

leblowl · 2024-05-07T17:03:44Z

packages/mobile/src/store/nativeServices/events/nativeServicesCallbacks.ts

+    console.log('nativeServicesCallbacksSaga stopping')
+    if (yield cancelled()) {
+      console.log('nativeServicesCallbacksSaga cancelled')
+    }


Just adding additional logging around all of these long-running sagas (anything that runs until cancelled... so anything with a takeEvery in it or other while (true) loop)

leblowl · 2024-05-07T17:05:41Z

packages/mobile/src/store/nativeServices/leaveCommunity/leaveCommunity.saga.ts

  // Restart backend
  yield* putResolve(app.actions.closeServices())
-
-  yield takeLeading(initActions.canceledRootTask.type, clearReduxStore)


We never want to run takeLeading or takeEvery more than once because, for example, in this case it will result in clearReduxStore being run multiple times. I moved this into the rootSaga instead.

leblowl · 2024-05-07T17:08:18Z

packages/mobile/src/store/root.saga.ts

+      fork(setupCryptoSaga),
+      fork(initMasterSaga),
+      fork(navigationMasterSaga),
+      fork(nativeServicesMasterSaga),


Running these multiple times without cancelling and restarting rootSaga results in multiple sagas being run for each event. I didn't see any need to cancel and restart rootSaga like we do with useIO, so I just used fork here.

leblowl · 2024-05-07T17:33:24Z

packages/backend/src/nest/connections-manager/connections-manager.service.ts

@@ -223,47 +223,51 @@ export class ConnectionsManagerService extends EventEmitter implements OnModuleI
    }
  }

-  public async closeAllServices(options: { saveTor: boolean } = { saveTor: false }) {
+  public async closeAllServices(
+    options: { saveTor: boolean; purgeLocalDb: boolean } = { saveTor: false, purgeLocalDb: false }


Feels a little clunky to add another parameter here since closeAllServices is doing a little more than just closing things now, but it seems like the simplest change without a larger refactor. Also if closeAllServices is purging local-db then maybe it should also purge data on the filesystem.

[question] Why do we need to add this parameter? Maybe I'm reading into the name localDbService too much, but it seems like the data that would be cleared by this.localDbService.purge() would be the same data that would be cleared by this.purgeData() called in leaveCommunity() since that purgeData() seems to holistically remove all the local db data from the filesystem. Adding a side effect to closeAllServices() seems like it would be a bit of a code smell.

I'm not really sure, if we are sure that LevelDB would be totally cleared by purgeData then I don't think this.localDbServer.purge() is required - I just rearranged what was already there.

If we close and then re-open the same LevelDB, is there data in memory that sticks around? Or does removing the LevelDB files clear everything?

https://groups.google.com/g/leveldb/c/MXIxcn8akd4?pli=1

From the link above "DestroyDB operation will be
faster for moderate to large databases (its implementation works
pretty much as Krzysztof Kowalczyk mentioned: it deletes all the
database files in the directory)." and then taking a look at the source code itself, looks like that is what's going on: https://github.com/google/leveldb/blob/068d5ee1a3ac40dabd00d211d5013af44be55bea/db/db_impl.cc#L1542

I just tested this and it appears to work.

leblowl · 2024-05-07T17:33:48Z

packages/backend/src/nest/connections-manager/connections-manager.service.ts

+  ) {
+    this.logger('Closing services')
+
+    await this.closeSocket()


Close the data server first

islathehut

This looks great!

adrastaea

I'm not entirely sure about the changes to the sagas, but I have some questions, some nitpicks, and some design reservations about the changes to the SocketService.

adrastaea · 2024-05-07T19:11:28Z

packages/backend/src/nest/socket/socket.service.ts

@@ -27,13 +27,17 @@ import { CONFIG_OPTIONS, SERVER_IO_PROVIDER } from '../const'
 import { ConfigOptions, ServerIoProviderTypes } from '../types'
 import { suspendableSocketEvents } from './suspendable.events'
 import Logger from '../common/logger'
+import { sleep } from '../common/sleep'


[nitpick] unused import

adrastaea · 2024-05-07T19:50:07Z

packages/backend/src/nest/socket/socket.service.ts

+      })
+    })
+
+    return () => sockets.forEach(s => s.destroy())


[design] This feels very unintuitive to me. attachListeners() doubling as closeSockets() feels like a violation of the single responsibility principle. Why can't we have a separate closeSockets() function? This would make the code easier to read and understand.

adrastaea · 2024-05-07T20:26:42Z

packages/backend/src/nest/socket/socket.service.ts

+        if (err) throw new Error(err.message)
+        resolve(count)
+      })
+    })
  }

  public listen = async (port = this.configOptions.socketIOPort): Promise<void> => {


[refactor] port is unused. We should either remove the parameter or use it in the function instead of hardcoding the port number.

adrastaea · 2024-05-07T20:48:48Z

packages/backend/src/nest/socket/socket.service.ts

+    // which didn't work for me.
+    const sockets = new Set<net.Socket>()
+
+    this.serverIoProvider.server.on('connection', conn => {


[bug] This seems like a confusion of the server and the socket io.

And in my testing never seems to be called

This gets called for me. There are two different sockets, the socket.io Socket (https://socket.io/docs/v4/server-api/#socket) and the Node net.Socket. I'm really just following the advice here: socketio/socket.io#1602 and it worked for me in cleaning up the hanging connections I was seeing, so I just went with that.

adrastaea · 2024-05-07T22:14:33Z

packages/backend/src/nest/socket/socket.service.ts

+        return
+      }
+
+      this.serverIoProvider.io.close(err => {


[question] Does this suffer from the same issue mentioned in the cleanup function stuck in attachListeners where you need to loop through each socket and disconnect? Also, does this remove all of the listeners that were attached in the attachListeners function? Could this cause a memory leak and duplication of actions if the SocketService is started back up after being close and the listeners are attached again?

Yea, this doesn't appear to close all connections properly. I don't think we need to remove the listeners ever. So those just get added once in the constructor.

adrastaea · 2024-05-07T22:18:57Z

packages/backend/src/nest/socket/socket.service.ts

+    // `this.serverIoProvider.server.listening` is sufficient.
+    if (this.listening) {
+      const numConnections = await this.getConnections()
+      this.logger('Failed to listen. Connections still open:', numConnections)


[question] Wouldn't it be better to just clean up the existing connections and proceed with listening?

I'm not sure. I thought about that, but there might be some trickiness with timing due to the listen and close being async. I'll have to look into it more, so just went with the simplest option for now.

adrastaea · 2024-05-07T22:21:51Z

packages/backend/src/nest/socket/socket.service.ts

      this.serverIoProvider.server.listen(this.configOptions.socketIOPort, '127.0.0.1', () => {
        this.logger(`Data server running on port ${this.configOptions.socketIOPort}`)
+        this.listening = true


[design] This seems like reinventing the wheel. I feel like this just introduces chances for desync between the state of the http server and the state of SocketService.listening. I'd rather see us use the built in property and properly handle the cases related to no longer listening, but not having properly cleaned up connections.

adrastaea · 2024-05-07T22:24:36Z

packages/backend/src/nest/socket/socket.service.ts

        resolve()
      })
+      this.logger('Disconnecting sockets')
+      this.closeSockets()


[question] Shouldn't we also detach listeners when closing sockets? I mentioned it before, but I'd rather see a dedicated closeSockets() and detachListeners() method.

I don't think we need to detach any listeners. They are just attached once and persist between opening and closing of the server.

adrastaea

Just nitpicks that don't necessarily need addressed. I am concerned about the move of attachListeners() from onModuleInit to the constructor, but I'm approving on the assumption that you have proven this doesn't cause any issues.

adrastaea · 2024-05-08T18:59:28Z

packages/backend/src/nest/socket/socket.service.ts

@@ -71,7 +76,7 @@ export class SocketService extends EventEmitter implements OnModuleInit {
    this.logger('init: Frontend connected')
  }

-  private readonly attachListeners = (): void => {
+  private readonly attachListeners = () => {


[nitpick] lost the return type

I think not specifying void is generally the standard TS convention

adrastaea · 2024-05-08T19:06:14Z

packages/backend/src/nest/socket/socket.service.ts

+
+    this.sockets = new Set<net.Socket>()
+
+    this.attachListeners()


[FYI] attachListeners() moved from onModuleInit() to the constructor. I'm not sure what side effects this would have. Might cause attachListeners to trigger before init?

Yea it does trigger before init and I think that's fine. I think it makes sense to put everything necessary to setup an object in the constructor and only using onModuleInit when necessary and it doesn't seem like attachListeners needs to be in onModuleInit.

adrastaea · 2024-05-08T19:08:06Z

packages/backend/src/nest/socket/socket.service.ts

        resolve()
      })
+
+      this.serverIoProvider.io.disconnectSockets(true)


[nitpick] I would put this in closeSockets, but mostly inconsequential.

…times and close backend server properly (#2499)

leblowl changed the title ~~fix: Reorder the closing of services and add additional logs~~ fix: Reorder the closing of services, prevent sagas running multiple times and close backend server properly May 6, 2024

leblowl force-pushed the fix-close-services-order branch from 7f61ba6 to 0060adc Compare May 6, 2024 22:16

Lucas Leblow added 4 commits May 7, 2024 10:41

fix: Reorder the closing of services and add additional logs

e1a97d4

We were closing LevelDB before closing OrbitDB which resulted in an exception.

fix: Sagas running multiple times and socket closing

c8718c5

Fixup

a3951e0

Fixup

bf0ebb5

leblowl force-pushed the fix-close-services-order branch from 467d988 to bf0ebb5 Compare May 7, 2024 16:47

leblowl commented May 7, 2024

View reviewed changes

Fixup

f7b370c

leblowl commented May 7, 2024

View reviewed changes

islathehut approved these changes May 7, 2024

View reviewed changes

Fixup

cec2c0d

adrastaea requested changes May 7, 2024

View reviewed changes

Lucas Leblow added 2 commits May 8, 2024 12:34

Fixups

de2c55a

Add patch

9758408

adrastaea approved these changes May 8, 2024

View reviewed changes

Lucas Leblow added 5 commits May 8, 2024 14:09

Fixup

06dee86

Fixup

c98b93e

Fix store.load

6a0a5a8

Fix mobile root sagas

05390e6

Fixup and update CHANGELOG

4483f71

leblowl mentioned this pull request May 9, 2024

Switching from home screen to channel switches back #2513

Open

leblowl merged commit 134fbcb into 2.2.0 May 9, 2024
22 of 26 checks passed

leblowl pushed a commit that referenced this pull request May 10, 2024

fix: Reorder the closing of services, prevent sagas running multiple …

1eef06c

…times and close backend server properly (#2499)

fix: Reorder the closing of services, prevent sagas running multiple times and close backend server properly #2499

fix: Reorder the closing of services, prevent sagas running multiple times and close backend server properly #2499

Conversation

leblowl commented May 1, 2024 • edited Loading

Pull Request Checklist

(Optional) Mobile checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leblowl May 7, 2024 • edited Loading

Choose a reason for hiding this comment

leblowl May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leblowl May 7, 2024 • edited Loading

Choose a reason for hiding this comment

leblowl May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leblowl May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islathehut left a comment

Choose a reason for hiding this comment

adrastaea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leblowl May 8, 2024 • edited Loading

Choose a reason for hiding this comment

adrastaea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leblowl May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leblowl commented May 1, 2024 •

edited

Loading

leblowl May 7, 2024 •

edited

Loading

leblowl May 7, 2024 •

edited

Loading

leblowl May 7, 2024 •

edited

Loading

leblowl May 7, 2024 •

edited

Loading

leblowl May 7, 2024 •

edited

Loading

leblowl May 8, 2024 •

edited

Loading

leblowl May 8, 2024 •

edited

Loading