feature/server-client-protocol-version-2 #1072

rex-schilasky · 2023-04-18T07:15:12Z

Pull request type

Please check the type of change your PR introduces:

Bugfix

What is the current behavior?
See issue #907 and pull request #918.

Fixes #905
Fixes #907

What is the new behavior?
Different client/server protocol versions introduced based on the work of @floroeske.

Client and server exchange their protocol version via registration layer.
Server will not start before any client registration is received.
Server starts with clients protocol version (currently 0 or 1).
A server can handle multiple clients with different protocol version.
A client can be connected to multiple server with different protocol versions.

Does this introduce a breaking change?

Yes

Public API changed /include/ecal/ecal_callback.h.

github-actions

clang-tidy made some suggestions

github-actions · 2023-04-18T07:43:08Z

ecal/core/src/service/asio_server.h

  std::string            request_;
  std::string            response_;
  std::vector<char>      packed_response_;

  enum { max_length = 64 * 1024 };
-  char data_[max_length];
+  char                   data_[max_length];


warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]

char data_[max_length]; ^

github-actions · 2023-04-18T07:43:08Z

ecal/core/src/service/asio_server.h

  std::string            request_;
  std::string            response_;
  std::vector<char>      packed_response_;

  enum { max_length = 64 * 1024 };
-  char data_[max_length];
+  char                   data_[max_length];


warning: use default member initializer for 'data_' [modernize-use-default-member-init]

ecal/core/src/service/asio_server.h:44:

- : socket_(io_service), data_{} + : socket_(io_service),

Suggested change

char data_[max_length];

char data_[max_length]{};

github-actions · 2023-04-18T07:43:09Z

ecal/core/src/service/asio_server.h

@@ -236,7 +357,7 @@ class CAsioServer
      delete new_session;


warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory]

delete new_session; ^

Additional context

ecal/core/src/service/asio_server.h:338: variable declared here

void handle_accept(CAsioSession* new_session, unsigned int version, ^

github-actions · 2023-04-18T07:43:09Z

ecal/core/src/service/ecal_tcpclient.cpp

+    return true;
+  }
+
+  std::vector<char> CTcpClient::PackRequest(const std::string& request)


warning: method 'PackRequest' can be made static [readability-convert-member-functions-to-static]

ecal/core/src/service/ecal_tcpclient.h:106:

- std::vector<char> PackRequest(const std::string &request); + static std::vector<char> PackRequest(const std::string &request);

…with protocol v0 and protocol v1

github-actions

clang-tidy made some suggestions

github-actions · 2023-04-19T13:09:19Z

ecal/core/src/mon/ecal_monitoring_impl.h

@@ -43,7 +43,7 @@ namespace eCAL
  {
  public:
    CMonitoringImpl();
-    ~CMonitoringImpl();
+    ~CMonitoringImpl() = default;


warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]

Suggested change

~CMonitoringImpl() = default;

~CMonitoringImpl() override = default;

github-actions · 2023-04-19T13:09:20Z

ecal/core/src/service/asio_server.h

  std::string            request_;
  std::string            response_;
  std::vector<char>      packed_response_;

  enum { max_length = 64 * 1024 };
-  char data_[max_length];
+  char                   data_[max_length];


warning: use default member initializer for 'data_' [modernize-use-default-member-init]

ecal/core/src/service/asio_server.h:45:

- : socket_(io_service), data_{} + : socket_(io_service),

Suggested change

char data_[max_length];

char data_[max_length]{};

github-actions · 2023-04-19T13:09:20Z

ecal/core/src/service/asio_server.h

@@ -236,7 +358,7 @@ class CAsioServer
      delete new_session;


warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory]

delete new_session; ^

Additional context

ecal/core/src/service/asio_server.h:339: variable declared here

void handle_accept(CAsioSession* new_session, unsigned int version, ^

KanonWY · 2023-04-20T03:59:17Z

ecal/core/src/service/asio_server.h

+        {
+          request_ += data.substr(bytes_used, bytes_transferred - bytes_used);
+        }
+


Consider a relatively rare case, suppose pack_1 and pack_2 are sent in rapid succession on the tcpclient side. If pack_1 is internally split for some reason, and then resend part of the internally split packets (tcp automatic retransmission), then the new packet actually contains the second half of pack_1 and the first half of pack_2, the current code implementation does not do anything to the first half of pack_2.
Of course, due to the nodelay socket option enabled on the sender side, this makes it difficult to reproduce the situation, but there is still a low probability of this problem.

I assume by pack_1 and pack_2 you mean two separate requests sent by the same client using the same TCP connection. I guess you are right it could happen, but I think only when you start two requests in parallel using two threads. I think if you send two requests from the same thread using the same client it would be processed sequentially. The first client call would block until it received the response from the server.

I think a more likely problem could be that for some reason not all data comes through and the server will wait forever to receive a complete payload. Then a new request might complete the payload receiving and corrupt the data.

First of all, thank you for your review - GREAT. I will check all in detail later.

floroeske

Cool, I'll try it out in the next few days.

floroeske · 2023-04-20T06:50:40Z

ecal/core/src/service/asio_server.h

+        {
+          request_ += data.substr(bytes_used, bytes_transferred - bytes_used);
+        }
+


I assume by pack_1 and pack_2 you mean two separate requests sent by the same client using the same TCP connection. I guess you are right it could happen, but I think only when you start two requests in parallel using two threads. I think if you send two requests from the same thread using the same client it would be processed sequentially. The first client call would block until it received the response from the server.

I think a more likely problem could be that for some reason not all data comes through and the server will wait forever to receive a complete payload. Then a new request might complete the payload receiving and corrupt the data.

floroeske · 2023-04-20T06:55:05Z

ecal/core/src/service/asio_server.h

+        if (header_.size() == sizeof(eCAL::STcpHeader) && header_request_size_ == 0)
+        {
+          eCAL::STcpHeader tcp_header;
+          memcpy(&tcp_header, header_.data(), sizeof(eCAL::STcpHeader));


I wonder if we should add a method to decode and check if the header is valid. e.g. version field in the header and maybe a checksum. If it's not valid cancel here and restart. The is a possibility that we can receive a rubbish header resulting in a wrong or very large payload size.

floroeske · 2023-04-20T06:59:11Z

ecal/core/src/service/asio_server.h

+    }
+    else
+    {
+      if ((ec == asio::error::eof) ||


One thing I couldn't get working successfully was adding timeouts to asio. As far as I understand this just handles connection loss, but not timeouts. Theoretically the server could hang there forever trying to receive a full payload. CTcpClient::ReceiveResponse has a timeout handler though I haven't tested it.

floroeske · 2023-04-20T07:24:41Z

ecal/core/src/service/ecal_tcpclient.cpp

@@ -241,7 +316,7 @@ namespace eCAL
      // read stream header (sync)
      else
      {
-        size_t bytes_read = m_socket->read_some(asio::buffer(&tcp_header, sizeof(tcp_header)));
+        size_t const bytes_read = m_socket->read_some(asio::buffer(&tcp_header, sizeof(tcp_header)));


Maybe same thing here. Validate header and check if psize_n makes sense.

floroeske · 2023-04-20T07:28:52Z

ecal/core/src/service/ecal_tcpclient.cpp

+    return true;
+  }
+
+  std::vector<char> CTcpClient::PackRequest(const std::string& request)


Wondering if PackRequest and PackResponse should live in core/src/service/ecal_tcpheader.h. Maybe one PackHeader() function would be enough.

github-actions

clang-tidy made some suggestions

ecal/core/src/ecal_def.h

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

floroeske · 2023-05-02T04:06:24Z

I've been running this branch for the last week on some of our devices and I haven't found any issues so far. I haven't tested the protocol backwards compatibility.

2 info logs changed to severity level debug2

github-actions

clang-tidy made some suggestions

github-actions · 2023-05-04T12:35:36Z

ecal/core/src/service/asio_server.h

  std::string            request_;
  std::string            response_;
  std::vector<char>      packed_response_;

  enum { max_length = 64 * 1024 };
-  char data_[max_length];
+  char                   data_[max_length]{};


warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]

char data_[max_length]{}; ^

github-actions · 2023-05-04T12:35:36Z

ecal/core/src/service/asio_server.h

@@ -236,7 +359,7 @@ class CAsioServer
      delete new_session;


warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory]

delete new_session; ^

Additional context

ecal/core/src/service/asio_server.h:340: variable declared here

void handle_accept(CAsioSession* new_session, unsigned int version, ^

…ature/server-client-protocol-version-2

The new implementation can already communicate with both V1 and V0 protocol clients

… introduces messages for v1 handshake

…primitive test

…p_client

…eed on fast machines

…ature/server-client-protocol-version-2

…e, again

… been stopped

…ature/server-client-protocol-version-2

FlorianReimold · 2023-09-20T06:04:36Z

I am finally handing this over to anybody who would like to review the changes 😉

…ature/server-client-protocol-version-2

rex-schilasky added 2 commits April 18, 2023 08:29

protocol version 0 & 1 introduced

9c467c2

clang-tidy fixes

7a9fbf0

github-actions bot reviewed Apr 18, 2023

View reviewed changes

rex-schilasky added 2 commits April 19, 2023 10:33

full upward and downward compatibility by starting 2 server sessions …

d9d8a98

…with protocol v0 and protocol v1

tcp port fixed to new v1

9c86df1

rex-schilasky mentioned this pull request Apr 19, 2023

Fix tcp server/client request #918

Closed

9 tasks

clang-tidy fixes

642a248

github-actions bot reviewed Apr 19, 2023

View reviewed changes

KanonWY reviewed Apr 20, 2023

View reviewed changes

floroeske reviewed Apr 20, 2023

View reviewed changes

KerstinKeller added this to the eCAL 5.12 milestone Apr 21, 2023

config switches added to activate/deactivate service protocols

835972a

github-actions bot reviewed Apr 22, 2023

View reviewed changes

ecal/core/src/ecal_def.h Outdated Show resolved Hide resolved

Update ecal/core/src/ecal_def.h

a997158

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

eCAL::STcpHeader extended with version and header size

f7bc1b5

2 info logs changed to severity level debug2

github-actions bot reviewed May 4, 2023

View reviewed changes

FlorianReimold marked this pull request as draft May 5, 2023 06:15

FlorianReimold added 12 commits May 5, 2023 11:32

Core CMake: Restructured source_groups of files for IDEs

a9b9f71

Merge branch 'master' of https://github.com/eclipse-ecal/ecal into fe…

134f98d

…ature/server-client-protocol-version-2

Intermediate state

7dbaccf

The new implementation can already communicate with both V1 and V0 protocol clients

Refactoring and a lot of debug output, especially for V1

3615f62

Couple of clang-tidy fixes

4572a43

Removed unused variables

7ec8402

Fixed bugs in v0 implementation and added many logging messages. Also…

06db8d0

… introduces messages for v1 handshake

Enabled tcp_nodelay option

d386c8a

removed old code and fixed logging

3e058d3

Moved tcp server implementation to an object library and added first …

ef77af1

…primitive test

Fixed compile issue

05817da

Fixed a TODO (weak_ptr instead of this) and started preparing asio_tc…

2c5a91f

…p_client

FlorianReimold added 21 commits August 2, 2023 18:27

Fixed many clang tidy warnings

6068b7d

Tests: Improved robustness on slow machines and improved execution sp…

d2306ed

…eed on fast machines

Solved many TODOs

b7f0148

Merge branch 'master' of https://github.com/eclipse-ecal/ecal into fe…

d43f8e7

…ature/server-client-protocol-version-2

Made CServiceClientImpl a shared_ptr

5583ac8

Fixed missing include

58ada8a

Fixed more TODOs

e40cf51

Added another test testing the non-parallel service calls

6c8125f

Added sample and Readme.md

4435a13

added readme and sampel

bd2f43f

Revised folder structure

5c57ccb

Added missing includes

74568a1

Added a lot of doxygen documentation

de9f8b2

Some refactoring

b6fde93

Removed unnecessary int return value from service callback

63c4598

Removed some TODOs

e0cf790

Increased timeouts in tests, so they hopefully won't fail due to thos…

e2ec436

…e, again

Added comments pointing to potential issues that can arise in the future

81fa6d5

Fixed potential deadlock when calling a service blocking after it had…

1bd7c4f

… been stopped

Merge branch 'master' of https://github.com/eclipse-ecal/ecal into fe…

7b52bc2

…ature/server-client-protocol-version-2

Added more documentation to Readme.md

135f3fc

FlorianReimold marked this pull request as ready for review September 20, 2023 06:03

FlorianReimold added 3 commits September 21, 2023 08:33

Updated HDF5 again

7405aac

A little refactoring

6232564

Merge branch 'master' of https://github.com/eclipse-ecal/ecal into fe…

67c713d

…ature/server-client-protocol-version-2

hannemn approved these changes Oct 12, 2023

View reviewed changes

Small refactoring

c1474c0

FlorianReimold merged commit 5ca0c8a into master Oct 13, 2023
14 checks passed

FlorianReimold deleted the feature/server-client-protocol-version-2 branch November 8, 2023 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/server-client-protocol-version-2 #1072

feature/server-client-protocol-version-2 #1072

rex-schilasky commented Apr 18, 2023 •

edited by FlorianReimold

Loading

github-actions bot left a comment

github-actions bot Apr 18, 2023

github-actions bot Apr 18, 2023

github-actions bot Apr 18, 2023

github-actions bot Apr 18, 2023

github-actions bot left a comment

github-actions bot Apr 19, 2023

github-actions bot Apr 19, 2023

github-actions bot Apr 19, 2023

KanonWY Apr 20, 2023

floroeske Apr 20, 2023

rex-schilasky Apr 20, 2023

floroeske left a comment

floroeske Apr 20, 2023

floroeske Apr 20, 2023

floroeske Apr 20, 2023

floroeske Apr 20, 2023

floroeske Apr 20, 2023

github-actions bot left a comment

floroeske commented May 2, 2023

github-actions bot left a comment

github-actions bot May 4, 2023

github-actions bot May 4, 2023

FlorianReimold commented Sep 20, 2023

	~CMonitoringImpl() = default;
	~CMonitoringImpl() override = default;

feature/server-client-protocol-version-2 #1072

feature/server-client-protocol-version-2 #1072

Conversation

rex-schilasky commented Apr 18, 2023 • edited by FlorianReimold Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Apr 18, 2023

Choose a reason for hiding this comment

github-actions bot Apr 18, 2023

Choose a reason for hiding this comment

github-actions bot Apr 18, 2023

Choose a reason for hiding this comment

github-actions bot Apr 18, 2023

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Apr 19, 2023

Choose a reason for hiding this comment

github-actions bot Apr 19, 2023

Choose a reason for hiding this comment

github-actions bot Apr 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

floroeske left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

floroeske commented May 2, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot May 4, 2023

Choose a reason for hiding this comment

github-actions bot May 4, 2023

Choose a reason for hiding this comment

FlorianReimold commented Sep 20, 2023

rex-schilasky commented Apr 18, 2023 •

edited by FlorianReimold

Loading