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

Server receiving data out of order (DataReceived method triggers out of order) #15

Closed
pha3z opened this issue May 29, 2020 · 15 comments
Closed

Comments

@pha3z
Copy link

pha3z commented May 29, 2020

Hey Joel,

I have an external system that I have no control over sending data to my TCP Server. I had to build the Server to conform to the client system pattern.

The client is supposed to send what it calls "blocks" of data and a complete sequence of blocks forms a complete message. Each block is prefixed with 2 bytes that indicate the block length. Max block size is 65536. If a block prefix says the length will be 65536, then at least one more block will follow. When a prefix says block size is less than 65536, then its the last block. Putting all the blocks together in order establishes the entire message.

The problem I'm having is that parts of the blocks are not always triggering the DataReceived method in correct order. Sometimes I receive a piece of a block in the byte[] from DataReceived.
Then I receive start of the block when DataReceive triggers again.... and finally the end of the block. I have no way of knowing which order the parts go in to properly assemble a complete block. (If the block parts happen to come in the right order, I have no trouble putting the blocks together to get the entire message.)

I can't tell what's going on closer to the raw TCP stack, which is quite a problem. What I expected is that the existing client app would break a block into TCP packets to send them, and Simple TCP Server would put the packets together in the correct order (even if they were received out of order) so that I'm guaranteed to at least get packets (and thereby parts of blocks) in order.

Do you have any inkling what might be going wrong? Have I made incorrect assumptions about what Simple Server should do?

@jchristn
Copy link
Owner

Hi @pha3z - yes, issue received and typing a (much longer than anticipated) response, wanted to let you know!

@jchristn
Copy link
Owner

jchristn commented May 29, 2020

Hi @pha3z - yes, what you're describing is the need for framing (which SimpleTcp doesn't do). SimpleTcp just has a single thread continually reading from a stream and punting the byte[] data to your app via an event.

Here's what I recommend you do to implement a simplistic framing mechanism (pseudo-code). SimpleTcp won't be able to accomplish this as is, but we may be able to craft a workable solution for you. The main reason being is that the stream reader is continually reading and firing events, and not giving you any input on how many bytes you actually want (which I think is a useful enhancement).

Assume that level of control is available to your app while reviewing this pseudocode.

  • Read two bytes from the network and convert to integer i
  • If i == 65536
    • True: read 65536 bytes from network, write into an output stream, and repeat
    • False: read i bytes from network, write into an output stream
  • Convert output stream to byte array and send to application

I believe something like this is what you want, correct?

@WizADSL
Copy link

WizADSL commented May 29, 2020

Hi @pha3z - yes, what you're describing is the need for framing (which SimpleTcp doesn't do). SimpleTcp just has a single thread continually reading from a stream and punting the byte[] data to your app via an event.

Here's what I recommend you do to implement a simplistic framing mechanism (pseudo-code). SimpleTcp won't be able to accomplish this as is, but we may be able to craft a workable solution for you. The main reason being is that the stream reader is continually reading and firing events, and not giving you any input on how many bytes you actually want (which I think is a useful enhancement).

Assume that level of control is available to your app while reviewing this pseudocode.

  • Read two bytes from the network and convert to integer i

  • If i == 65536

    • True: read 65536 bytes from network, write into an output stream, and repeat
    • False: read i bytes from network, write into an output stream
  • Convert output stream to byte array and send to application

I believe something like this is what you want, correct?

I could certainly use that.

@pha3z
Copy link
Author

pha3z commented May 29, 2020

The logic sounds exactly right to me. I think that would fix the problem.

But this network stuff really confuses me. Are you saying that if Simple TCP knew how many bytes to expect as a complete network block/frame, it would be able to ensure the data is provided to application in the correct order?

In order for that to even hold true, would Simple TCP at least need a guarantee that the first packet (with the first two bytes) arrives before any other packets. Or is the issue of "data framing" separate from the issue of packet arrangement; are they two separate layers? As I explained in my original question, I saw one occurrence where a small piece of a block was provided from DataReceived() before the first piece of the block was provided from a second firing of the event. I presume that happened because packets arrived out of order, and Simple TCP just handed over the packet that arrived first instead of waiting to assemble the packets together in order. Is that correct?

If I'm understanding this correctly and the issue is really an issue of packet ordering, it seems like a more "generalized" fix would be to offer a "Ordered Packets" option so that data is always pushed to the application in order the sender originally sent it. Is that feasible?

@jchristn
Copy link
Owner

Hi @pha3z SimpleTcp has, on the client side, a single thread reading from the stream using a buffer of a fixed size. Once it reads data, it passes it to the application. It isn't aware of where an application-layer message begins, ends, etc. That's where framing comes in - SimpleTcp doesn't do it (WatsonTcp does, but it requires WatsonTcp on both ends). On the server side, SimpleTcp has a single thread per client and each thread reads only from the stream for that particular client.

TCP provides sequence numbers to ensure that data arrives in order, even if the packets containing the data do not. SimpleTcp doesn't get in the way of that, and leaves that to the underlying operating system. See: https://packetlife.net/blog/2010/jun/7/understanding-tcp-sequence-acknowledgment-numbers/

So I'm not sure how anything would ever arrive out of order. Event invocations are also synchronous, meaning when DataReceived is invoked, the DataReceiver is blocked. If you're seeing data blocks out of order based on the invocations of your DataReceived event handler, then they are either being a) sent out of order or b) the underlying OS is not re-ordering according to the sequence numbers.

Please take a look at this gist - I hacked down the SimpleTcp client to something that gives full control of how many bytes to read to the calling application. I haven't done it for the server (more complicated, unless you're only ever going to have a single client connected).

Let me know if something like this would be useful. Look primarily at the three public methods Connect, Send, and Read. https://gist.github.com/jchristn/cf518c17d4fa5ebc4b9fe501d06fb1f7

As I mentioned this is very easy to do client-side, server-side is harder unless you're ok with only having a single connected client.

@pha3z
Copy link
Author

pha3z commented May 29, 2020

So I'm not sure how anything would ever arrive out of order. Event invocations are also synchronous, meaning when DataReceived is invoked, the DataReceiver is blocked. If you're seeing data blocks out of order based on the invocations of your DataReceived event handler, then they are either being a) sent out of order or b) the underlying OS is not re-ordering according to the sequence numbers. - jchristn

P > That's precisely what I'm describing and why I was very confused. To be absolutely clear, yes: I logged at least one event where the client should have sent a block of data in this form: ABC. DataReceived() fired on the server 3 different times, and the order of data I got from the firings was: BAC.

Are you 100% positive there isn't some bug that could cause DataReceived invocations to occur out of order? Its very hard to believe that the 3rd party client is sending packets out of order... and even harder to believe the OS is not re-ordering properly... its Ubuntu server.

@pha3z
Copy link
Author

pha3z commented May 29, 2020

I think I know what the problem is. Its in my handler:

private async Task<bool> DataReceived(string ipPort, byte[] data)
{
            //_log?.Debug("[TCP_SERVER] Data received");
            await _dataReceivedHandler(ipPort, new ArrayBufferReader<byte>(data));
            return true;
}

Since I'm calling my own internal handler asynchronously, that means .net can divert control back to the original caller, which allows the DataReceiver to fire immediately again for the next packet. Then there's a race condition. And the second invocation happens to beat the first invocation.

In order to assemble the packets properly, all code called from the DataReceived() method has to be synchronous. Do you concur with this?

@jchristn
Copy link
Owner

Hi @pha3z, to your question:

In order to assemble the packets properly, all code called from the DataReceived() method has to be synchronous. Do you concur with this?

Going synchronous would be one way to solve the problem. And in this type of case I would recommend it because there's a relationship between the data components (A, B, C) that you're reading from the network.

If I were to craft another NuGet package that provided a client and a server, exposing APIs similar to what I showed in the gist, would that be useful to you? This is probably something I could do in a pretty short amount of time.

@pha3z
Copy link
Author

pha3z commented May 30, 2020

I don't think it would be helpful to me, because I already have the entire application logic in place to do the Data Framing. It works perfectly fine when I get the data in the right order. I just wasn't 100% sure how the TCP packet reception worked under the hood and I thought that was the cause of me getting data out of order.

What seemed odd to me though is that the DataReceived event handler for Simple TCP server is expected to return a Task. But after some thought, I suppose sometimes someone might intentionally WANT to react asynchronously to event data, in which case the event handler would need to be defined as asynchronous. Maybe this is one of the reasons async programming often gets confusing. I think I mistakenly thought the event handler is async, so I should be calling things from it asynchronously as a pattern.

@jchristn
Copy link
Owner

jchristn commented May 30, 2020

What version are you using? The latest (2.x) uses EventHandler instead of callbacks.

i.e.

public event EventHandler<DataReceivedFromClientEventArgs> DataReceived;

And the implementation:

_Server.DataReceived += DataReceived;

and

static void DataReceived(object sender, DataReceivedFromClientEventArgs e)
{
    Console.WriteLine("[" + e.IpPort + "]: " + Encoding.UTF8.GetString(e.Data));
}

@pha3z
Copy link
Author

pha3z commented May 30, 2020

That makes way more sense! :)
My project is still on version 1.1.7.

I suppose I'll have to read the change releases now and catch up.

@jchristn
Copy link
Owner

Ok - please give it a whirl and let me know if this helps at all. It shouldn't be a terribly painful upgrade.

But just note, this won't solve your framing problem...

Please let me know if this at least solves the out of order issue. Cheers, Joel

@pha3z
Copy link
Author

pha3z commented May 30, 2020

Succeeded with the upgrade. Code works solid now. 💯 Thank you so much for your help!

@jchristn
Copy link
Owner

Glad to hear my friend! BTW I've started working on another library that will give more explicit control over reads etc. I'll post the link to it here when it's ready. Cheers @pha3z

@jchristn
Copy link
Owner

Hi @pha3z not sure if this is of any use/value to you, but I modified SimpleTcp to give the consuming application explicit control over when data is read. This will allow much easier build outs of state machines but has some draw backs, one of which being less-than-immediate disconnect detection (since there is no background thread continually attempting to read data).

Please let me know if there is any interest in trying this out as potentially a better fit:

Repo: https://github.com/jchristn/cavemantcp
NuGet: https://www.nuget.org/packages/CavemanTcp/

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