-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: master
Are you sure you want to change the base?
Conversation
@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; |
There was a problem hiding this comment.
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:
- 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. - 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).
- 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.
There was a problem hiding this comment.
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.
Any update for this issue? I have same issue with this issue. |
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 theMQTTAsync_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