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

Add send queue to TCPObject #658

Closed
wants to merge 2 commits into
base: development
from

Conversation

Projects
None yet
6 participants
@jamesu
Contributor

jamesu commented May 19, 2014

Implements a fix for issue #655 by adding a simple send queue to TCPObject. This also adds a SimpleDataQueue class to the engine (let me know if I somehow missed an already-existing streaming queued buffer class!).

All connected TCPObject instances are processed in the run loop to ensure all data has been sent. The send queue is iterated until either the queue is empty or the TCP layer stops sending bytes through.

Due to the varying mysterious nature of send buffers in TCP implementations it is advised that this code is tested simulating a pretty bad connection to ensure the buffer is actually made use of. In most cases the code will at least act like the old send() as a call to send the data is issued immediately.

@crabmusket crabmusket added this to the 3.6 milestone May 19, 2014

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket May 19, 2014

Contributor

Appreciated! I'm putting this in 3.6 for now, but if we're unable to test it adequately we may bump it to 3.7. I'll review it in detail later.

Contributor

crabmusket commented May 19, 2014

Appreciated! I'm putting this in 3.6 for now, but if we're unable to test it adequately we may bump it to 3.7. I'll review it in detail later.

@dottools

This comment has been minimized.

Show comment
Hide comment
@dottools

dottools May 19, 2014

This implementation looks completely wrong. First of all TCPObject::processSendBuffer() assumes either it's all or nothing of data up to 4096 long, while Net::send() uses BSD socket send() function that has the exact same rules as fwrite() and other standard I/O which is never assume the operating system accepted the entire data when no error, instead check the returned byte count to know how much it really did accept and and offset that by how much you tried to send, then you'll know what bytes are remaining. Your simple queue is wrong too. It should have a get head data and seek/skip like functions so that TCPObject::processSendBuffer() can get a pointer to the data and also count of used bytes in that one block of memory. That is then passed directly to Net::send() and then seek/skip the bytes that were actually 'written'.

Secondly, your Simple queue implementation is very complicated for what it's trying to do. If you really want good performance you'll want a ring buffer implementation, however ring buffers don't normally support buffer size expansion cause they're static size for simplistic sake. Alternatively you could use std::vector directly for dynamic memory allocation, however it is expensive on popping front units as it'll move everything one unit forward each time, so kind of bad for this usage case, however unless you only use std::vector.reserve() and std::vector.size() functions and directly manipulate the buffer yourself.

Third, TCPObject's send() method shouldn't be a send and forget call. Instead it really should do the same thing that BSD socket's send() does and just tell the user how much it really did send and then it's up to the user to use schedule() or other means to retry the unsent data. This will avoid the requirement of storing up to large size files of data in a send queue causing Torque's memory usage to balloon up. Besides it's just bad practice in general to read an entire file into memory anyway.

dottools commented May 19, 2014

This implementation looks completely wrong. First of all TCPObject::processSendBuffer() assumes either it's all or nothing of data up to 4096 long, while Net::send() uses BSD socket send() function that has the exact same rules as fwrite() and other standard I/O which is never assume the operating system accepted the entire data when no error, instead check the returned byte count to know how much it really did accept and and offset that by how much you tried to send, then you'll know what bytes are remaining. Your simple queue is wrong too. It should have a get head data and seek/skip like functions so that TCPObject::processSendBuffer() can get a pointer to the data and also count of used bytes in that one block of memory. That is then passed directly to Net::send() and then seek/skip the bytes that were actually 'written'.

Secondly, your Simple queue implementation is very complicated for what it's trying to do. If you really want good performance you'll want a ring buffer implementation, however ring buffers don't normally support buffer size expansion cause they're static size for simplistic sake. Alternatively you could use std::vector directly for dynamic memory allocation, however it is expensive on popping front units as it'll move everything one unit forward each time, so kind of bad for this usage case, however unless you only use std::vector.reserve() and std::vector.size() functions and directly manipulate the buffer yourself.

Third, TCPObject's send() method shouldn't be a send and forget call. Instead it really should do the same thing that BSD socket's send() does and just tell the user how much it really did send and then it's up to the user to use schedule() or other means to retry the unsent data. This will avoid the requirement of storing up to large size files of data in a send queue causing Torque's memory usage to balloon up. Besides it's just bad practice in general to read an entire file into memory anyway.

@jamesu

This comment has been minimized.

Show comment
Hide comment
@jamesu

jamesu May 19, 2014

Contributor

@dottools

To address your points

"This implementation looks completely wrong. First of all TCPObject::processSendBuffer() assumes either it's all or nothing of data up to 4096 long"

All processSendBuffer does is write data from the mSendQueue in 4kb chunks. It uses the return value from Net::send in order to determine how many bytes were actually "written" to the TCP stream.

"check the returned byte count to know how much it really did accept and and offset that by how much you tried to send"

This is what the code does. Provided no error is emitted by Net::send it will destroy the data in mSendQueue ( https://github.com/jamesu/Torque3D/blob/tcpobjectfix/Engine/source/app/net/tcpObject.cpp#L437 )

"Your simple queue is wrong too. It should have a get head data and seek/skip like functions "

copyToBuffer copies data from the buffer without expending the queue bytes. expendBuffer actually destroys the data from the head position. write appends data onto the tail of the queue. This is loosely based on the boost asio streambuf class.

"Secondly, your Simple queue implementation is very complicated for what it's trying to do. If you really want good performance you'll want a ring buffer implementation"

Not the best code In the world, I admit. Based on the original design constraints however it worked. Admittedly performance wasn't a huge consideration since I wasn't making a bit torrent client or some kind of elaborate network renderer.

"Third, TCPObject's send() method shouldn't be a send and forget call."

That's debatable. Do you really expect scripters to need to handle partial TCP sends with convoluted schedule calls? A simple send("HELLO"); call would need a fair bit of scripting overhead in order to properly handle data which isn't sent.

In any case, this improvement is not designed to be able to send large files through the send script call (that's impractical especially considering you can't send '\0'). Rather it's designed to handle the case where for whatever reason besides a fatal error data simply isn't sent fully.

Originally this was used in conjunction with the file resource which allows files to be streamed through TCPObject using a FileStream. In this case the normal send() calls were processed after the file had finished sending, thus one of the main reasons why its stuffed in a send queue. If you really want I could just make send return the number of bytes sent and make everyone do it the hard way! ;)

Contributor

jamesu commented May 19, 2014

@dottools

To address your points

"This implementation looks completely wrong. First of all TCPObject::processSendBuffer() assumes either it's all or nothing of data up to 4096 long"

All processSendBuffer does is write data from the mSendQueue in 4kb chunks. It uses the return value from Net::send in order to determine how many bytes were actually "written" to the TCP stream.

"check the returned byte count to know how much it really did accept and and offset that by how much you tried to send"

This is what the code does. Provided no error is emitted by Net::send it will destroy the data in mSendQueue ( https://github.com/jamesu/Torque3D/blob/tcpobjectfix/Engine/source/app/net/tcpObject.cpp#L437 )

"Your simple queue is wrong too. It should have a get head data and seek/skip like functions "

copyToBuffer copies data from the buffer without expending the queue bytes. expendBuffer actually destroys the data from the head position. write appends data onto the tail of the queue. This is loosely based on the boost asio streambuf class.

"Secondly, your Simple queue implementation is very complicated for what it's trying to do. If you really want good performance you'll want a ring buffer implementation"

Not the best code In the world, I admit. Based on the original design constraints however it worked. Admittedly performance wasn't a huge consideration since I wasn't making a bit torrent client or some kind of elaborate network renderer.

"Third, TCPObject's send() method shouldn't be a send and forget call."

That's debatable. Do you really expect scripters to need to handle partial TCP sends with convoluted schedule calls? A simple send("HELLO"); call would need a fair bit of scripting overhead in order to properly handle data which isn't sent.

In any case, this improvement is not designed to be able to send large files through the send script call (that's impractical especially considering you can't send '\0'). Rather it's designed to handle the case where for whatever reason besides a fatal error data simply isn't sent fully.

Originally this was used in conjunction with the file resource which allows files to be streamed through TCPObject using a FileStream. In this case the normal send() calls were processed after the file had finished sending, thus one of the main reasons why its stuffed in a send queue. If you really want I could just make send return the number of bytes sent and make everyone do it the hard way! ;)

@MusicMonkey5555

This comment has been minimized.

Show comment
Hide comment
@MusicMonkey5555

MusicMonkey5555 Jul 28, 2014

Contributor

I am using the TCPObject to make a custom http server system in torquescript since from what I read the HTTP object is limited and broken for the most part. Plus it gives me more control and I can do stuff like use Websockets.

Anyway I added a sendFile method to TCPObject that simply takes a path, reads every byte of the data into a vector of U8 and then sends that vector. The issue is the entire file is never sent and there is no return from send or any way of knowing when how much was sent. So the header can't be changed to notify the browser that it is being chunked instead.

There could be other issues similar to this. And although it isn't a direct bug with your code, it is a limitation. There needs to be both a return added to send (console method and engine) of how much data was sent, maybe even a heads up for how much will be sent. And then some sort of callback.

Or any other solution you think would work.

If you need more info or code I can try and put some of it up. Or post in a comment or something. But it's pretty simple.

Contributor

MusicMonkey5555 commented Jul 28, 2014

I am using the TCPObject to make a custom http server system in torquescript since from what I read the HTTP object is limited and broken for the most part. Plus it gives me more control and I can do stuff like use Websockets.

Anyway I added a sendFile method to TCPObject that simply takes a path, reads every byte of the data into a vector of U8 and then sends that vector. The issue is the entire file is never sent and there is no return from send or any way of knowing when how much was sent. So the header can't be changed to notify the browser that it is being chunked instead.

There could be other issues similar to this. And although it isn't a direct bug with your code, it is a limitation. There needs to be both a return added to send (console method and engine) of how much data was sent, maybe even a heads up for how much will be sent. And then some sort of callback.

Or any other solution you think would work.

If you need more info or code I can try and put some of it up. Or post in a comment or something. But it's pretty simple.

@MusicMonkey5555

This comment has been minimized.

Show comment
Hide comment
@MusicMonkey5555

MusicMonkey5555 Jul 28, 2014

Contributor

Also as bit more of a organization thing, although if changes where needed it would make compile times longer. I would split simpleDataQueue.h into two files (.h & .cpp). That is an easy enough change though.

Contributor

MusicMonkey5555 commented Jul 28, 2014

Also as bit more of a organization thing, although if changes where needed it would make compile times longer. I would split simpleDataQueue.h into two files (.h & .cpp). That is an easy enough change though.

@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Aug 24, 2014

Contributor

@jamesu, thx for your work :)

After a initial review (not tested code):

  • I'm OK with outBytesWritten on low level functions and send&forget on TCPObject
  • Even if SimpleDataQueue can be a bit complicated for our needs, I OK with this class, i only have a few comments.
    • We can change memory block allocation from mBlockSize to (mBlockSize + bytesToWrite)? For reduce number of allocations when bytesToWrite are bigger or near same size of mBlockSize.
    • We can move SimpleDataQueue functions implementation outsize class declaration on header file? For easy read class.
Contributor

LuisAntonRebollo commented Aug 24, 2014

@jamesu, thx for your work :)

After a initial review (not tested code):

  • I'm OK with outBytesWritten on low level functions and send&forget on TCPObject
  • Even if SimpleDataQueue can be a bit complicated for our needs, I OK with this class, i only have a few comments.
    • We can change memory block allocation from mBlockSize to (mBlockSize + bytesToWrite)? For reduce number of allocations when bytesToWrite are bigger or near same size of mBlockSize.
    • We can move SimpleDataQueue functions implementation outsize class declaration on header file? For easy read class.
@jamesu

This comment has been minimized.

Show comment
Hide comment
@jamesu

jamesu Aug 24, 2014

Contributor

@LuisAntonRebollo
(mBlockSize + bytesToWrite) ? There is a slight problem with that as it as the block size is assumed to be fixed. Doing otherwise would further complicate the read/write logic. If you want to use this approach it would be better to just create a whole new class to use a single dRealloc'd block of memory.

Implementations can be moved, though since this class was more of a one-off only used by TCPObject I didn't see much point in sticking them in a cpp.

Contributor

jamesu commented Aug 24, 2014

@LuisAntonRebollo
(mBlockSize + bytesToWrite) ? There is a slight problem with that as it as the block size is assumed to be fixed. Doing otherwise would further complicate the read/write logic. If you want to use this approach it would be better to just create a whole new class to use a single dRealloc'd block of memory.

Implementations can be moved, though since this class was more of a one-off only used by TCPObject I didn't see much point in sticking them in a cpp.

@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Aug 24, 2014

Contributor

Yes, i see the problem about change block size. We have more important issues to dedicate time.

Sorry, I think I misspoke. I meant to move the implementation of the functions but on header file. We don't need a cpp for this.

Contributor

LuisAntonRebollo commented Aug 24, 2014

Yes, i see the problem about change block size. We have more important issues to dedicate time.

Sorry, I think I misspoke. I meant to move the implementation of the functions but on header file. We don't need a cpp for this.

@Areloch

This comment has been minimized.

Show comment
Hide comment
@Areloch

Areloch Sep 25, 2014

Contributor

As there doesn't seem to be a real consensus on this about the implementation, and I don't think any of the current SC are knowledgeable enough to step in to take a shot at a implementation that assuages concerns, I'm going to put this down for 3.7 instead of 3.6 to give a bit more time to be percolated on as it doesn't seem to be game breaking, just rather inconvenient.

Contributor

Areloch commented Sep 25, 2014

As there doesn't seem to be a real consensus on this about the implementation, and I don't think any of the current SC are knowledgeable enough to step in to take a shot at a implementation that assuages concerns, I'm going to put this down for 3.7 instead of 3.6 to give a bit more time to be percolated on as it doesn't seem to be game breaking, just rather inconvenient.

@Areloch Areloch modified the milestones: 3.7, 3.6 Sep 25, 2014

@jamesu

This comment has been minimized.

Show comment
Hide comment
@jamesu

jamesu Sep 25, 2014

Contributor

After much consideration, due to lack of serious interest I will be removing this pull request. (hereby rescinding it as a contribution)

Contributor

jamesu commented Sep 25, 2014

After much consideration, due to lack of serious interest I will be removing this pull request. (hereby rescinding it as a contribution)

@jamesu jamesu closed this Sep 25, 2014

@jamesu jamesu referenced this pull request Feb 12, 2015

Merged

Tcp additions #1186

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment