Skip to content

Respect connect_timeout #169

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

Merged
merged 2 commits into from
Sep 12, 2022
Merged

Respect connect_timeout #169

merged 2 commits into from
Sep 12, 2022

Conversation

ansd
Copy link
Contributor

@ansd ansd commented Aug 30, 2022

Prior to this commit, when the client created a TCP connection with the
server and the server did not respond with a CONNACK (for example
because the server is blocked due to overload), the client was stuck in
state waiting_for_connack without respecting the configured
connect_timeout.

@zmstone
Copy link
Member

zmstone commented Aug 31, 2022

Hi @ansd
Thank you for the PR.
Could you please rebase and add a line to changelog.md ?

Prior to this commit, when the client created a TCP connection with the
server and the server did not respond with a CONNACK (for example
because the server is blocked due to overload), the client was stuck in
state waiting_for_connack without respecting the configured
connect_timeout.
@ansd
Copy link
Contributor Author

ansd commented Aug 31, 2022

Thanks @zmstone.
I rebased and added a line to the changelog.

@zmstone
Copy link
Member

zmstone commented Sep 3, 2022

The problem, seems to be the use of event_timeout()
here:

emqtt/src/emqtt.erl

Lines 779 to 780 in fdd9959

{next_state, waiting_for_connack,
add_call(new_call(connect, From), NewState), [Timeout]};

and here:
{next_state, waiting_for_connack, NewState, [Timeout]};

i.e. any event will cause the timer to be cancelled.
adding another event_timeout() after handing the event may seem to fix the issue, but maybe not the optimal way.

Should maybe use a state_timeout() instead. i.e. the total time allowed to stay at the waiting_for_connack state.

@zmstone
Copy link
Member

zmstone commented Sep 3, 2022

A side-note:
Arguably, the timeout should include both the time to establish the TCP connection, and the time to send CONNECT and receive CONNACK.
However, if we are to fix it, it becomes a behavior change which is slightly incompatible to the previous versions.

As @zmstone pointed out, using a state_timeout() for state
waiting_for_connack makes more sense than an event_timeout()
because any event will cause an event_timeout() to be cancelled.
The total time in state waiting_for_connack should not exceed the
configured connect_timeout (no matter what events are received).
@ansd
Copy link
Contributor Author

ansd commented Sep 4, 2022

Very good point and thanks for the explanation @zmstone: a state_timeout() is much better here. I pushed one more commit.

@zmstone zmstone merged commit 7a3d0dd into emqx:master Sep 12, 2022
ansd added a commit to rabbitmq/rabbitmq-server that referenced this pull request Sep 21, 2022
Given that emqx/emqtt#169 has been merged
and a new tag has been set on emqx/emqtt,
we do not need the fork ansd/emqtt anymore.
mergify bot pushed a commit to rabbitmq/rabbitmq-server that referenced this pull request Sep 21, 2022
Given that emqx/emqtt#169 has been merged
and a new tag has been set on emqx/emqtt,
we do not need the fork ansd/emqtt anymore.

(cherry picked from commit 307e673)
mergify bot pushed a commit to rabbitmq/rabbitmq-server that referenced this pull request Sep 21, 2022
Given that emqx/emqtt#169 has been merged
and a new tag has been set on emqx/emqtt,
we do not need the fork ansd/emqtt anymore.

(cherry picked from commit 307e673)
(cherry picked from commit 7fffaac)
mergify bot pushed a commit to rabbitmq/rabbitmq-server that referenced this pull request Sep 22, 2022
Given that emqx/emqtt#169 has been merged
and a new tag has been set on emqx/emqtt,
we do not need the fork ansd/emqtt anymore.

(cherry picked from commit 307e673)
(cherry picked from commit 7fffaac)
(cherry picked from commit a52fb2f)
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