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

Reduce the delay of ASYNC connection #1458

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TiejunMS
Copy link

@TiejunMS TiejunMS commented Mar 8, 2024

Resolve the issue mentioned in #1430

Move the MQTT layer connect from receive thread to send thread as the TCP/Websocket connection is done in send thread. The root cause of the 2 seconds delay mentioned in issue is that, MQTT connection needs to wait two rounds of timeout (1s) polling a socket to be ready. For the first time, Socket_getReadySocket is polling events for first socket before second socket is connected and added to fds_read. For the second time, the second socket is connected, but there is no incoming data. By moving the MQTTAsync_connecting to send thread, MQTT connection can start immediately after underlayer connection is established.

I also removed 100ms sleep after checking Socket_getReadySocket. Please double check if this makes sense. Or else, each packet handling may encounter 100ms delay when no socket is ready. This is critical for applications pursuing performance.

Another change is the timeout reducing from 1000ms to 10ms in the receive thread. It will make the receiving thread more responsive while has little impact on the CPU usage.

Here is the result with modified timeout in sample code. mqtt_sample.txt

====== Creating MQTT client ======
====== creating MQTT connect ======
====== Successful connection ====== 
====== Connect Time elpased is 1 ms ======

====== Creating MQTT client ======
====== creating MQTT connect ======
====== Successful connection ====== 
====== Connect Time elpased is 11 ms ======

====== Creating MQTT client ======
====== creating MQTT connect ======
====== Successful connection ====== 
====== Connect Time elpased is 9 ms ======

@TiejunMS
Copy link
Author

TiejunMS commented Mar 8, 2024

@icraggs , just so you know, I'm in @ericwol-msft 's team. We are doing evaluation of open source Rust MQTT clients.

@@ -2031,7 +2034,7 @@ thread_return_type WINAPI MQTTAsync_receiveThread(void* n)

if (sock == 0)
continue;
timeout = 1000L;
timeout = 10L;

Choose a reason for hiding this comment

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

I don't think this modification is correct.
The first time the function is entered, the call to MQTTAsync_cycle(&sock, timeout, &rc); uses timeout = 10.
If this loop has to be executed the second time around, then timeout is increased to 1000.
The question really is why was 1000 picked?

Let's consider the following:

  1. What is really this timeout value?
    Feel free to look at static MQTTPacket* MQTTAsync_cycle(SOCKET* sock, unsigned long timeout, int* rc).
    It's used in the Socket_getReadySocket(0, (int)timeout, socket_mutex, &rc1); call.
  2. In the body of SOCKET Socket_getReadySocket(int more_work, int timeout, mutex_type mutex, int* rc), timeout is used to set a local variable timeout_ms, which as a good programming practice is initialized at the top of the function (happens to be 1000).
  3. Now, there are two versions of the SOCKET Socket_getReadySocket(int more_work, int timeout, mutex_type mutex, int* rc) function. Although they are different, they seem to imply that a slowdown in operation might be desired if getting a socket did not succeed the first time around.

Copy link
Author

Choose a reason for hiding this comment

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

I agree this change is not the best practice. This can be considered to be a temporary fix. The root cause of connection delay is that, not all socket operations are done in Socket_getReadySocket(). TCP connection is established in send thread. Even when it is completed, receive thread will continue wait until timeout. The best solution to avoid unexpected timeout for TCP connection is to move that logic (TCP connection) to receive Thread.

@JinyongJeong-TIM
Copy link

Any update for this issue? I have same issue with this issue.

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.

3 participants