AtherEnergy / rumqtt

Pure rust mqtt cilent
The Unlicense
203 stars 72 forks source link

Notifications on connection state changes and reconnection handling #142

Closed flxo closed 5 years ago

flxo commented 5 years ago

Hi,

I scratched the notifications of the connection state. Instead of a plain crossbeam_channel::Receiver the notifications thing returned is struct that wraps this and a oneshot channel for the sender to detect if the receiving side is dropped.

Next I refactored the reconnection handling (think the AfterFirstSuccess was broken anyway ;-)) for pushing Errors along with Notification::Disconnected.

This PR tries to fix #139 and #82.

During implementing this patch I touhght the one or other time that merging ConnectError and ClientError is worth to consider. For the user a single Error type might also be more convenient.

I'm open for a discussion on the detail and expect comments!

Thanks,

@flxo

tekjar commented 5 years ago

Hi. Sorry for the late response. Kinda sick. I'll be able to check this on Thursday. I've started some clean up as well in connection_refactor branch along with test cases.

Is it possible to make this pull request only about notifications? I've made some changes to reconnection handling as well. Also it'll make my life easier reviewing and visiting back the feature in future

flxo commented 5 years ago

Hi, hope you're doing better!

The reason for the refactoring is that I'd love to include the Error in the Disconnect notification. Currently the error handling is interrupted at some points e.g. this Result<_, bool> thing and in order to get the Error where I needed it I had to shuffle things around.

Anyway. I started to rebase this patch on top of your connect_refactor brach. Its WIP here.

Hope I can finish that one till end of the week and will leave you a note here - or if everything is fine I'll do a PR against connection_refactor. Will try to not touch the reconnections behaviour :-)

cheers,

@flxo

tekjar commented 5 years ago

Will try to not touch the reconnections behaviour :-)

Thank you :)

flxo commented 5 years ago

Closing this in favour of #143.