Skip to content

Comments

✨ Add drop filter#256

Open
hhalex wants to merge 5 commits intotokio-rs:mainfrom
hyli-org:feat/add_helper_to_drop_messages_on_condition
Open

✨ Add drop filter#256
hhalex wants to merge 5 commits intotokio-rs:mainfrom
hyli-org:feat/add_helper_to_drop_messages_on_condition

Conversation

@hhalex
Copy link

@hhalex hhalex commented Dec 16, 2025

Hello,

I added a configurable drop filter in Sim/Topology that runs before messages enter the network for any protocol/direction, plus helpers to set/clear it and a UDP test covering selective drops.

Copy link
Contributor

@mcches mcches left a comment

Choose a reason for hiding this comment

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

This seems like a nice change, but I'm curious if the existing mechanism could be modified instead?

https://docs.rs/turmoil/latest/turmoil/struct.SentRef.html

This could be updated to mark for drop.

src/host.rs Outdated
Some(sock.assign_seq())
}

pub(crate) fn rollback_send_seq(&mut self, pair: SocketPair, seq: u64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this behavior without having retransmit implemented. Can you explain the intention and consequences here?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry I made a few tests before, i cleaned it

@hhalex
Copy link
Author

hhalex commented Dec 17, 2025

This seems like a nice change, but I'm curious if the existing mechanism could be modified instead?

https://docs.rs/turmoil/latest/turmoil/struct.SentRef.html

This could be updated to mark for drop.

I figured out there is a simpler way to achieve this indeed. I did not add a drop_filter at the link level, may be I should ? I did not need for my use cases.

@mcches
Copy link
Contributor

mcches commented Dec 29, 2025

Sorry for the review delay. I was out for a bit. What's the state of this PR? I've got a couple of qs:

  • Why wouldn't drop on SentRef work for this?
  • How can we drop a tcp segment and the stream still works? I assume the seqno would be off.

@hhalex
Copy link
Author

hhalex commented Jan 5, 2026

No problem, thanks for your time, and happy new year. PR in discussion, i am using the feature for my needs on a forked version.

One clarification on intent: the drop filter is meant to drop an applicative‑level message at the network boundary, not a TCP‑level segment (even if segment‑level dropping would be a useful feature in its own right).

In the current model, SentRef always points at a Protocol item (src/envelope.rs), which for TCP is a single Segment. So by the time you’re in SentRef, you’re already dealing with TCP segments, not higher‑level “messages.” That’s why the filter is applied before enqueue — it’s the only place to interpret “message” at that higher level.

Since here a message = a segment, i should be ok to deal with at this segment level?

edit: I tried to minimize the enqueue/deliver signatures modifications, what do you think?

@hhalex hhalex force-pushed the feat/add_helper_to_drop_messages_on_condition branch from 81876cc to 424cbf5 Compare January 5, 2026 16:58
@mcches
Copy link
Contributor

mcches commented Jan 6, 2026

One clarification on intent: the drop filter is meant to drop an applicative‑level message at the network boundary, not a TCP‑level segment (even if segment‑level dropping would be a useful feature in its own right).

There is no application-level message concept in turmoil. Each AsyncWrite happens to be 1:1 with a "packet", but that was just a simplification. We are in the process of making this more realistic, with MTU and such, so this can't be an assumption going forward.

How can we drop a tcp segment and the stream still works? I assume the seqno would be off.

I'm still curious about this. drop_filter_skips_data_without_corrupting_stream appears to break TCP semantics.

I really think a more general run until predicate structure is better suited here. That could provide the packet that caused the predicate to succeed, and we can hold, release, or drop in that hook. I'd prefer to hold off on new public APIs while I'm doing this refactoring so that we don't expose something, and then have to take it back or change it in the new version.

@hhalex
Copy link
Author

hhalex commented Jan 7, 2026

There is no application-level message concept in turmoil. Each AsyncWrite happens to be 1:1 with a "packet", but that was just a simplification.

Got it, may be I should implement it somewhere else then, may be at the frame level.

We are in the process of making this more realistic, with MTU and such, so this can't be an assumption going forward.

Very nice.

I'm still curious about this. drop_filter_skips_data_without_corrupting_stream appears to break TCP semantics.

Yes, I reread it more carefully, and this test should not pass ? The stream should just stall, very strange, I'll dig a bit.

I really think a more general run until predicate structure is better suited here. That could provide the packet that caused the predicate to succeed, and we can hold, release, or drop in that hook. I'd prefer to hold off on new public APIs while I'm doing this refactoring so that we don't expose something, and then have to take it back or change it in the new version.

Yeah, I realize that dropping messages worked for me as stalling streams were dropped if they are inactive for too long and triggered reconnections, making application level messages drop as i needed, without suffering from stalling streams. Dropping tcp segments might be interesting, but what i want to test is the robustness of the protocol I am building upon it (could be udp too)

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.

2 participants