fix: set default network timeout as Duration::MAX instead of zero.

[Ddystopia]

Type of change

Bug fix (non-breaking change which fixes an issue)

Checklist:

• Formatted with cargo fmt
• Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.

Closes #945.
Closes #948.

[swanandx]

can we use set_keepalive method instead of passing value to new()? ( like change suggested in the issue comment )

also, we should add comment on top of it mentioning why we are setting it to connection_timeout_ms.

[Ddystopia]

I find it the most logical to pass to the new. Are there some reasons you don't want this?

[swanandx]

I find it the most logical to pass to the new. Are there some reasons you don't want this?

• minimal /no changes in API ( non-breaking change )
• no overhead of keeping it consistent in multiple places ( like in this PR )
• set_keepalive is method intended for this purpose, we are going to overwrite it anyways

[Ddystopia]

I find it the most logical to pass to the new. Are there some reasons you don't want this?

• minimal /no changes in API ( non-breaking change )
• no overhead of keeping it consistent in multiple places ( like in this PR )
• set_keepalive is method intended for this purpose, we are going to overwrite it anyways

As far as I can see, Network is not a part of public api. About consistency, are you sure that in other places we do not need setting keepalive? And set_keepalive accepts whole seconds, so we would need to round up those milliseconds from conf?

Maybe having Duration::MAX as a default would suit you more? Anyway there is a timeout in mqtt_connect.

[swanandx]

And set_keepalive accepts whole seconds, so we would need to round up those milliseconds from conf?

fair point, ideally there should be different config entry but would be too much, and if we want to use connection_timeout_ms we should round it up to bigger value, like 7100 should round to 8. ( don't know how complicated it will be )

Maybe having Duration::MAX as a default would suit you more? Anyway there is a timeout in mqtt_connect.

reason for using Duration::ZERO was so that it would be easier to spot bugs ( like the one we are fixing rn ), it is MAX it will just block indefinitely and would be harder to spot.

but using connection_timeout_ms in bridge doesn't make sense ( as we should either be using keepalive or waiting forever like before #784 ).

so yeah, let's go with Duration::MAX as default and also add comment on top of it mentioning that it would be overwritten by keepalive value in connect packet, otherwise we would wait indefinitely until we get a packet / network error when we call read.

[swanandx]

thanks for the PR @Ddystopia ! 🚀