codingteam / emulsion

XMPP ↔ Telegram bridge
MIT License
33 stars 3 forks source link

XMPP room connection handler #18

Closed ForNeVeR closed 5 years ago

ForNeVeR commented 7 years ago

Consider this scenario:

  1. Client connects to XMPP, but not yet entered the room
  2. Client receives a message from Telegram
  3. Client tries to send a message to the room and fails
  4. Client connects to the XMPP room

In that case, the message will be lost. We need to prevent cases like this. Probably with small async magic when connecting to the room.

ForNeVeR commented 5 years ago

Blocked on #29.

ForNeVeR commented 5 years ago

It is a cause of real incidents such as #54. I'm raising the priority.

ForNeVeR commented 5 years ago

Alrighty, I'll dump some current thoughts here. I want to create some sort of async wrapper around the XMPP client, so we'll be able to call something like that:

let client = AsyncClient.create()
async {
  do! client.Connect server
  do! client.SignIn credentials
  do! client.EnterRoom roomSettings
  do! client.Run()
}

Where Run() should call some sort of event handlers where each handler itself may be async, too.

There're problems with error handling though.

Some of the methods may throw exceptions (e.g. SignIn will throw if it cannot sign in, EnterRoom will throw if it cannot enter the room), that's simple and we know how to handle that. But some methods establish a context where an error may be thrown at any moment.

For example, after I execute client.Connect(), the server may disconnect me literally at any moment, thus terminating the whole workflow (and the disconnect should terminate the whole async workflow actually). And client.EnterRoom() establishes the room context which should be terminated after the bot leaves the room (e.g. by being kicked).

This concept of contexts that may be terminated at any moment with an error is complex, and I don't know how to properly encode that. Probably may be encoded using child asyncronous workflows in F#? I don't know because I didn't worked with them much.

In my earlier notes, I was thinking about something like that:

client.SignIn(sessionLifetime => {
  client.EnterRoom(sessionLifetime, roomLifetime => {
    client.SendRoomMessage(roomLifetime, "text");
  });
});

Where each stage has its own "lifetime", the lifetimes are nested and terminated properly. But I'm not sure about that now.

Also, there may be problems with our current approach because of the race conditions when accessing the client output stream. I'll need to think about that, too.

ForNeVeR commented 5 years ago

@ForNeVeR please investigate thread safety of our current solution and raise another issue about that. Probably we'll need to solve that first, but this is an issue about XMPP connection/room session API.

ForNeVeR commented 5 years ago

Blocked on #66. Current work on the new API is stashed into the branch feature/18.async-xmpp.

ForNeVeR commented 5 years ago

@ForNeVeR please investigate thread safety of our current solution and raise another issue about that. Probably we'll need to solve that first, but this is an issue about XMPP connection/room session API.

Thanks for the question, I didn't initially thought about that.

The SharpXMPP itself is thread-safe if the library client doesn't allow multiple simultaneous calls to the Send method.

So the architecture proposed here should be thread-safe as well. Our current usage is not thread safe.

ForNeVeR commented 5 years ago

@ForNeVeR,

The SharpXMPP itself is thread-safe if the library client doesn't allow multiple simultaneous calls to the Send method.

Further analysis has shown that not to be true. Sometimes, SharpXMPP will send its own data to the server (e.g. when answering some IQ queries). Looks like it isn't safe by design. Please investigate more and fix.

ForNeVeR commented 5 years ago

@ForNeVeR please investigate thread safety of our current solution and raise another issue about that.

Here you are: #78.