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

feat: TCP Transport Layer #1636

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: TCP Transport Layer #1636

wants to merge 1 commit into from

Conversation

Wincent01
Copy link
Member

@Wincent01 Wincent01 commented Oct 13, 2024

TCP Transport Layer

Scope

This PR seeks to add an alternative TCP transport protocol. It works under the specifications of lcdr's tcpudp shim dll. The protocol initially specified the use of UDP for unreliable packets, but this is disabled in the dll due to port issues. This feature PR will assume this disabled state is permanent; UDP for unreliable packets will not be enabled.

Current status

  • Optionally compiled additional TCP transport layer.
  • Config to enable it.
  • Tested and functional with 2 clients.
  • Removed unused RakNet replica manager and id manager. We've got our own replica manager since pre-open-source.
  • Utilizes async boost behavior.

Refactor structure

  • dServer is not an adaptor for the transport layer.
  • Multiple transport layers that implement a interface.
  • No significant change to the dServer interface.

Todo

  • Figure out how to do ping calculations.
  • Fix crashes on universe shutdown.
  • Test TLS on a VPS.
  • Remove unnecessary logging.
  • Test with lots of clients.
  • Maybe evaluate performance vs raknet.
  • Finish "master" to "manager" naming refactor (I'm going to be that guy, this is as good of a time as any).
  • Fix CI

* Optionally compiled additional TCP transport layer.
* Config to enable it.
* Tested and functional with lcdr's tcpudp dll, udp being disabled in the dll due to port issues.
* Removed unused RakNet replica manager and id manager. We've got our own replica manager since pre-open-source.
* Utilizes async boost behavior.

Todo:
* Figure out how to do ping calculations.
* Fix crashes on universe shutdown.
* Test TLS on a VPS.
* Remove unnecessary logging.
* Test with lots of clients.
* Finish "master" to "manager" naming refactor.
Copy link
Collaborator

@EmosewaMC EmosewaMC left a comment

Choose a reason for hiding this comment

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

cool stuff, some comments about bounds checking but overall looks cool

}

void TcpPeer::DeallocatePacket(Packet* packet) {
delete[] packet->data;
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider checking that packet->data is not a nullptr and also that the packet passed into here is a valid pointer as well

}

void TcpTransportLayer::DeallocatePacket(Packet* packet) {
delete[] packet->data;
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider checking that packet->data is not a nullptr and also that the packet passed into here is a valid pointer as well

@@ -0,0 +1,272 @@
#define _VARIADIC_MAX 10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a particular reason for this define?

Comment on lines +103 to +105
for (const auto& c : m_Password) {
bitStream.Write<uint8_t>(c);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider WriteBytes here

Comment on lines +202 to +205
bitStream.Write<uint8_t>(packet->data[1]);
bitStream.Write<uint8_t>(packet->data[2]);
bitStream.Write<uint8_t>(packet->data[3]);
bitStream.Write<uint8_t>(packet->data[4]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could access undefined heap memory if data is < 5 in size, i would consider bounds checks here on a packet

Comment on lines +28 to +32
std::size_t operator()(const SystemAddress& sysAddr) const {
const std::size_t hash1 = sysAddr.binaryAddress;
const std::size_t hash2 = sysAddr.port;
return (hash1 | (hash2 << 32));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

on platforms where size_t is 32 bits, this will result in just being hash1 always. a 32 bit shift on a 4 byte value also is not a valid operation which is worth considering for a different hash value

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