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

fragmentation issue #238

Closed
kollapsderwellenfunktion opened this issue Sep 6, 2019 · 5 comments
Closed

fragmentation issue #238

kollapsderwellenfunktion opened this issue Sep 6, 2019 · 5 comments

Comments

@kollapsderwellenfunktion

when i send packets < fragment_size everything works as expected.

when i send a packet larger than than that (let's say 4000 bytes) using reliable_unordered, i can see on wireshark that the client sends multiple packets, so fragmentation on the client side seems to work.

but on the server side i just get nothing with sizes larger than fragment_size.

server side looks like this

let mut socket = Socket::bind_with_config(SERVER, Config::default()).map_err(|err| ServerError)?;
    let (sender, receiver) = (socket.get_packet_sender(), socket.get_event_receiver());
    let _thread = thread::spawn(move || socket.start_polling());

    'recv_loop: loop {
        if let Ok(event) = receiver.recv() {
            match event {
                SocketEvent::Packet(packet) => {
                    let msg = packet.payload();

i use laminar 0.3 from crates on linux. client and server are on localhost, mtu on lo is 65536

i cloned the repo, and run the tests with the tester features, all tests seem to pass.

it's most likely just an oversight from on my side....

@TimonPost
Copy link
Owner

TimonPost commented Sep 6, 2019

You are right, it has some problems. I just validated that. I can see that the fragments do arrive at the server, but they aren't released from the buffer storing the fragments until they all arrived. I will have a further look at why that is.

sending payload of 4000 bytes + header bytes

receveived lenght: 1041
receveived lenght: 1033
receveived lenght: 1033
receveived lenght: 937

fragmented FragmentHeader { sequence: 0, id: 0, num_fragments: 4 }
acked Some(AckedPacketHeader { seq: 0, ack_seq: 65535, ack_field: 0 })
fragmented FragmentHeader { sequence: 0, id: 1, num_fragments: 4 }
acked None
fragmented FragmentHeader { sequence: 0, id: 2, num_fragments: 4 }
acked None
fragmented FragmentHeader { sequence: 0, id: 3, num_fragments: 4 }
acked None

@TimonPost
Copy link
Owner

TimonPost commented Sep 6, 2019

Okay, I did some more research. This behavior has to do that the connection is not initialized and thus a new connection is created each time a packet is sent. This behavior is to prevent DOS attack (#194). To set up a valid connection you have to make sure you're sending an initial packet, the server should then respond to this packet, by doing that your saying I trust this packet of this client. When sending the response on the server a connection is inserted in a hashmap and state of that connection can be stored - which is, in this case, the fragmentation information.

As spoken in #174, this behavior should be improved by a proper handshaking implementation and will be implemented by #156.

related issue: #174

To recap:

  1. Send 'unreliable packet' from client to server
  2. Receive packet on the server, respond to the client with an 'unreliable packet' when receiving that packet.

Now you should be able to have fully reliable communication with fragmentation.

Please let us know if you have more questions

@kstrafe
Copy link
Contributor

kstrafe commented Sep 7, 2019

@TimonPost We should investigate - instead of a handshake method - a method to vet packets as they are received using a handler function:

socket.manual_poll(time, |unknown_pkt| -> Acceptance { /* code to accept a connection/deny */ });

This could make the users less confused, as there's an explicit lambda that you use to add a connection to the connection table.

@kollapsderwellenfunktion
Copy link
Author

i added a handshake, and it works now, thank you very much for your efforts.

@TimonPost
Copy link
Owner

Great, I think this issue can be closed.

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

No branches or pull requests

3 participants