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.

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

Already on GitHub? Sign in to your account

馃悰 Bug Report: Realtime.subscribe method doesn't dispatch any error if try to subscribe without connection. #131

Open
2 tasks done
FernandoUFS opened this issue Mar 8, 2023 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@FernandoUFS
Copy link

馃憻 Reproduction steps

When using realtime.subscribe without connection it's not possible detect if it succeeded or failed, because the error of connection is not dispatched to outside of the sdk.

The try catch is never fired nor the stream.onError nor the stream.onDone when trying to subscribe.
Example:

try {
  RealtimeSubscription _messagesRealtimeSubscription = realtime.subscribe(['databases.chat.collections.${widget.group}.documents']);

  _chatSubscription = _messagesRealtimeSubscription.stream.listen((event) {
    Log.w(event.toString(), prefix: 'chat');
  }, onError: (e, st) {
    Log.e(e.toString(), st: st, prefix: 'chat');
  }, onDone: () {
    Log.e('Connection closed', prefix: 'chat');
  });
} catch (e, st) {
  Log.e(e, st: st, prefix: 'chat');
}

馃憤 Expected behavior

It should be possible to catch connection error and it should be possible to be sure that the connection was stablished.

When I say "to be sure that the connection was stablished" I mean that knowing if the stream had an error only solves one part of the problem. This is because I cannot be sure that the connection was successful simply by expecting not to receive an error.

Maybe the subscribe method being a Future I can know when the connection was stablished by awaiting.
For instance:

try {
    await realtime.subscribe(['databases.chat.collections.${widget.group}.documents']);
    log('success');
    showSuccessToUser();
} catch (e, st){
    log(e.toString);
    showErrorToUser();
    startAutoReconnectApproach();
}

馃憥 Actual Behavior

It actually never returns any errors in any way. The onDone function is only called if the connection was successfully established before and then the connection dies. However, if you try to subscribe without a connection, no error is dispatched.

馃幉 Appwrite version

Version 2.0.x

馃捇 Operating system

MacOS

馃П Your Environment

flutter doctor

Doctor summary (to see all details, run flutter doctor -v):
[鉁揮 Flutter (Channel stable, 3.7.6, on macOS 13.1 22C65 darwin-arm64, locale en-BR)
[鉁揮 Android toolchain - develop for Android devices (Android SDK version 33.0.0)
[鉁揮 Xcode - develop for iOS and macOS (Xcode 14.2)
[鉁揮 Chrome - develop for the web
[鉁揮 Android Studio (version 2022.1)
[鉁揮 VS Code (version 1.76.0)
[鉁揮 Connected device (2 available)
[鉁揮 HTTP Host Availability

pubspec.yml

dependencies:
  appwrite: ^8.3.0

Device

Galaxy S22 with Android 13

馃憖 Have you spent some time to check if this issue has been raised before?

  • I checked and didn't find similar issue

馃彚 Have you read the Code of Conduct?

@FernandoUFS FernandoUFS added the bug Something isn't working label Mar 8, 2023
@stnguyen90
Copy link

@FernandoUFS thanks for creating this issue! 馃檹馃徏 I think this problem might be here:

Future.delayed(Duration.zero, () => _createSocket());

If _createSocket() throws an exception, it won't be handled.

@lohanidamodar, what are your thoughts on how to proceed with this?

@lohanidamodar
Copy link
Member

@FernandoUFS thanks for creating this issue! 馃檹馃徏 I think this problem might be here:

Future.delayed(Duration.zero, () => _createSocket());

If _createSocket() throws an exception, it won't be handled.

@lohanidamodar, what are your thoughts on how to proceed with this?

may be an await the Future can solve the problem?

FernandoUFS pushed a commit to FernandoUFS/appwrite-sdk-for-flutter that referenced this issue Mar 15, 2023
@FernandoUFS
Copy link
Author

@lohanidamodar I could solve this issue by two ways.

First way: The await worked, but to work I had to change the method subscribeTo from realtime_mixin.dart to return a Future<RealtimeSubscription> then, the same thing for Realtime.subscribe and its implementations. This causes a breaking change because the changing of the return type, everyone would have to do await realtime.subscribe now.
I committed the changes in a fork: FernandoUFS@a0825c8

Second way: This line is the one causing the error when trying connecting without connection:

_websok = await getWebSocket(uri);

Using a try catch in that point we can send the error to the streams. But also it would be necessary send a success stream, so we can be sure that the connection was stablished.

I tested with this code, and I could get the error in the realtimeSubscription stream onError.

try {
      if (_websok == null) {
        _websok = await getWebSocket(uri);
        // Create a RealtimeMessage and send to the channels informing that the connection was success.
        _lastUrl = uri.toString();
      } else {
        if (_lastUrl == uri.toString() && _websok?.closeCode == null) {
          return;
        }
        await _closeConnection();
        _lastUrl = uri.toString();
        _websok = await getWebSocket(uri);
      }
    } catch (e) {
      for (var list in _channels.values) {
        for (var stream in list) {
          stream.sink.addError(AppwriteException('Error connecting to server'));
        }
      }
    }

@lohanidamodar
Copy link
Member

Thank you @FernandoUFS would you like to make a PR against https://github.com/appwrite/sdk-generator ? If not, no worries, we will look into it as well.

@FernandoUFS
Copy link
Author

Hi @lohanidamodar, as you can see, I was able to solve the problem in two ways, but I think the first way (the code in the repository) is more intuitive. However, this causes a breaking change, because now everyone will need to use await realtime.subscribe for the code to work. So, I would prefer you analyze which is the best option. Thank you.

@lohanidamodar
Copy link
Member

Thank you @FernandoUFS yes, we would not want to break the interface as well as would want to keep it consistent with other SDKs. So giving back the error on onError for streams is the better option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants