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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send a multi device sent transcript content #38

Merged
merged 12 commits into from
Jan 28, 2021

Conversation

gferon
Copy link
Collaborator

@gferon gferon commented Nov 5, 2020

@gferon gferon changed the title Send a multi device sent transcript content WIP:Send a multi device sent transcript content Nov 5, 2020
@gferon gferon changed the title WIP:Send a multi device sent transcript content Send a multi device sent transcript content Nov 5, 2020
@gferon gferon marked this pull request as draft November 5, 2020 21:59
@rubdos rubdos mentioned this pull request Nov 7, 2020
@gferon gferon force-pushed the multi-device-sent-transcript-content branch from f0c4ed1 to 9090248 Compare November 11, 2020 20:18
@gferon gferon mentioned this pull request Nov 13, 2020
2 tasks
rubdos added a commit that referenced this pull request Nov 15, 2020
This can probably go away when fixing multidevice transcripts, i.e. #38
@gferon gferon force-pushed the multi-device-sent-transcript-content branch from 26de436 to 1676d43 Compare November 24, 2020 09:38
.gitignore Show resolved Hide resolved
Copy link
Member

@rubdos rubdos left a comment

Choose a reason for hiding this comment

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

So, the reason you drop mut is because create_encrypted_message and partners are called concurrently (because you want to drive them with join_all), and that would yield interleaved mutable borrows.

Some remarks:

  • I am still in favour of keeping the most generic interface (which is with mut as restriction)
  • While concurrently sending the individual messages does make sense, I can imagine it doesn't provide much benefit in practice.
pub struct Foo;

impl Foo {
    async fn fun(&mut self) -> Result<(), ()> {
        Ok(())
    }
    
    async fn some_concurrent(&mut self) {
        let multiple = (0..4).map(|_| self.fun());
        
        let results = futures::future::join_all(multiple).await;
    }
}

#[tokio::main]
async fn main() {
    let results = Foo.some_concurrent().await;
}

... rightfully yields

error: captured variable cannot escape `FnMut` closure body
 --> src/main.rs:9:39
  |
9 |         let multiple = (0..4).map(|_| self.fun());
  |                                     - ^^^^^^^^^^ returns a reference to a captured variable which escapes the closure body
  |                                     |
  |                                     inferred to be a `FnMut` closure
  |
  = note: `FnMut` closures only have access to their captured variables while they are executing...
  = note: ...therefore, they cannot allow references to captured variables to escape

On the other hand, Rust by default assumes that the async fn fun's Future needs to live for the lifetime of &mut self, which is not necessarily true:

pub struct Foo;

impl Foo {
    fn fun<'a>(&'a mut self) -> impl std::future::Future<Output=Result<(), ()>> + 'static {
        async { Ok(()) }
    }
    
    async fn some_concurrent(&mut self) {
        let multiple = (0..4).map(|_| self.fun());
        
        let results = futures::future::join_all(multiple).await;
    }
}

#[tokio::main]
async fn main() {
    let results = Foo.some_concurrent().await;
}

I wonder whether it's possible to re-engineer the try_send_message method to use a shared mutable reference. On the other hand, making a custom self parameter (like async fn try_send_message(self: RefCell<&mut Self>, ...) doesn't necessarily make things better here, especially since that trickles down into implementors. Even worse would probably be to use a futures::lock::Mutex for this. I wonder whether there's a (near-)free type to solve this kind of issue...

libsignal-service-actix/examples/registering.rs Outdated Show resolved Hide resolved
libsignal-service/src/sender.rs Outdated Show resolved Hide resolved
libsignal-service/src/sender.rs Outdated Show resolved Hide resolved
libsignal-service/src/sender.rs Outdated Show resolved Hide resolved
@gferon gferon changed the title Send a multi device sent transcript content WIP: Send a multi device sent transcript content Nov 27, 2020
@gferon
Copy link
Collaborator Author

gferon commented Nov 30, 2020

So, the reason you drop mut is because create_encrypted_message and partners are called concurrently (because you want to drive them with join_all), and that would yield interleaved mutable borrows.

Yes, this is exactly the reason why I wanted to drop the mutable borrow. For now I'll drop it entirely and keep it as it is. (also: sending multiple requests at the same time actually got me rate limited a few times).

@gferon gferon changed the title WIP: Send a multi device sent transcript content Send a multi device sent transcript content Nov 30, 2020
@gferon
Copy link
Collaborator Author

gferon commented Jan 12, 2021

@rubdos I think this is more or less in a OK state, I kind of lost track on the way and the change is rather big. I haven't been able to test group sending yet, and will do soon, but I think you can already give a look to the changes since your last review.

@gferon gferon force-pushed the multi-device-sent-transcript-content branch from 556df3f to be34f4a Compare January 25, 2021 09:24
@gferon gferon marked this pull request as ready for review January 28, 2021 13:24
@rubdos
Copy link
Member

rubdos commented Jan 28, 2021

If it's tested (including group) I think this is good to go.

@gferon gferon merged commit 11d1d51 into whisperfish:master Jan 28, 2021
@gferon gferon deleted the multi-device-sent-transcript-content branch January 28, 2021 13:32
rubdos added a commit to whisperfish/whisperfish that referenced this pull request Jan 30, 2021
@rubdos
Copy link
Member

rubdos commented Jan 30, 2021

[master aad0dbf] libsignal_service now handles group messages and multi-device transcript
 2 files changed, 28 insertions(+), 65 deletions(-)

Saved less code than I hoped. Meh :'-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

delete_all_sessions on EndSession Send group messages
3 participants