Skip to content

Conversation

@link2xt
Copy link
Collaborator

@link2xt link2xt commented Apr 18, 2024

Previously test did not trigger
deletion of ephemeral messages
and worked because clear_events() did not
remove just emitted events from send_text_msg.

Fixes #5470

…ages

Previously test did not trigger
deletion of ephemeral messages
and worked because clear_events() did not
remove just emitted events from `send_text_msg`.
@link2xt link2xt requested a review from Simon-Laux April 18, 2024 01:27
@link2xt
Copy link
Collaborator Author

link2xt commented Apr 18, 2024

Generally speaking clear_events() is a bad idea. Repeating what I said in 1:1 chat with @Simon-Laux:

Don't think you can reliably do it, there is always a chance that a new event will arrive just after you finish emptying it. You can implement something similar to consume_events in legacy python, but it is not reliable as it may exit too early because the event you are waiting for has not been read yet as stdio-reading loop did not get resources or deltachat-rpc-server did not get CPU.

This seems to be what happened, clear_events() did not clear all events.

chat.set_ephemeral_timer(&alice, crate::ephemeral::Timer::Enabled { duration: 1 })
chat.set_ephemeral_timer(&alice, crate::ephemeral::Timer::Enabled { duration: 60 })
.await?;
alice
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this necessary? Maybe not bad to check though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is to ensure we consume all events up to this one, including all chatlist events related to creation of group chat. Otherwise we may these events for events that we expect after deletion of ephemeral message. Better consume as many events as we can reliably expect as possible to have "checkpoints".

/// Avoid using this function if you can
/// by waiting for specific events you expect to receive.
pub async fn clear_events(&self) {
while let Ok(_ev) = tokio::time::timeout(Duration::from_secs(1), self.recv()).await {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can it be achieved by emitting a special test event and then waiting for it? Or there may be events that are emitted asynchronously?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that this task may simply not get resources and move events too slowly:
https://github.com/deltachat/deltachat-core-rust/blob/65822e53e669900d21f2c361458c1aa61851f480/src/test_utils.rs#L367-L376

Better way would be to get rid of this task so event cannot be in the process of being moved from one channel to another by the time we call clear_events(). But I don't know how much refactoring it is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually it is a problem also in cases when test crashes/panics, this may lose events and you don't get them in printout then. Maybe makes sense to file it as a bug. If not for this task, clear_events would work reliably even without 1 second timeout.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is now a PR #5478 with a fix for Rust clear_events.
For Python we indeed need a special event and wait for it, this is going to be implemented in #5477

@link2xt link2xt merged commit ff0d506 into main Apr 18, 2024
@link2xt link2xt deleted the link2xt/ephemeral-chatlist-flaky branch April 18, 2024 02:32
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.

events::chatlist_events::test_chatlist_events::test_update_after_ephemeral_messages is flaky

3 participants