Feature: Connection status notifications and notifications drop handling

[flxo]

This is a follow up on #142 without any change on the reconnection behaviour.

Check channel state before sending notifications and distibute connection state changes. The notification Receiver is wrapped into a struct that contains a oneshot Receiver that allows the client to check whether the notification struct is dropped or not. Upon disconnections the corresponding Error is sent.

[flxo]

lets think about merging ConnectError and NetworkError. I think that makes live easier for users and obsoletes ConnectError::NetworkError(_) which I needed to have a distinct type for Notification::Disconnected(_).

[flxo]

The test story is here is bad - I just added a very very basic one.

Any thoughts about kind of integration tests with a "real" broker? Especially for testing the connection state changes this is definitely more convenient than mocked unit tests...

[flxo]

One more note: I didn't increment the version which is kinda needed because of the API change. This can be done when eventually this branch is merged.

[manifest]

@flxo Thanks for working on that. We've just run into that issue and here is already a solution :-)

[manifest]

@tekjar Could we please merge this one and bump the crate version? This feature is quite important for our case.

[tekjar]

Sorry for the delay on this. I'll try to close this today

[tekjar]

Left some small nits. So AFAIU you are using oneshot channel along with crossbeam channel for synchronization. Give we anyway lose the convenience of crossbeam select with the wrapper, why not just use futures channel?

[tekjar]

Coming to your questions

lets think about merging ConnectError and NetworkError

We'll do this in the next PR

Any thoughts about kind of integration tests with a "real" broker? Especially for testing the connection state changes this is definitely more convenient than mocked unit tests

Integration tests with a real broker would be good but I thought we should also simulate bad networks, disconnections and half-open connections to continuously test out corner cases. But we can start with any available broker and later work on rumqttd to simulate those conditions. Gave a shot to toxiproxy but too many bugs

I didn't increment the version which is kinda needed because of the API change. This can be done when eventually this branch is merged

I'll increment when I merge this to master :)

[flxo]

Left some small nits. So AFAIU you are using oneshot channel along with crossbeam channel for synchronization. Give we anyway lose the convenience of crossbeam select with the wrapper, why not just use futures channel?

I don't see any drawback for using the futures channel since I'm in most of my project anyway in "future" context. You can still use the crossbeam select stuff with the impl Deref for Notifications that gives access to the crossbeam part. We can also consider to make this pub.

[tekjar]

Cool. Let's merge this after those small changes. I can make those changes myself after merging if you are busy

[TotalKrill]

Might i inquire for any news on this, after skimming through this PR, this seems like it would give users such as me ( with devices on bad networks a chance to react on network changes ) a chance to react to those network changes.

[tekjar]

@TotalKrill Couldn't spend any time on this crate now. Will most probably resume the work in June

[flxo]

@TotalKrill As you might have noticed there are a couple of open points that are not super easy to solve or decide. I'm using this branch in an experimental project and it works so far in that specific scenario. Comments are welcome!

[TotalKrill]

@flxo alright, I might have to take a look into that. A semi-related question though, how are topic subscriptions being handled during disconnect/connect?

Right now I have noticed that long running connections are not receiving subscription notifications after a while. I am suspecting this is due to a disconnect/reconnect. My preferred behavior would be to resubscribe to topics as well on reconnects.

[flxo]

As far as I know resubscriptions upon a reconnect is a open topic. See #85. Probably for now you have to do that on your own.

[TotalKrill]

the clean_session(false) seems to solve it as long as the broker doesnt die

[tekjar]

@TotalKrill

As @flxo mentioned, I'll need some consensus on automatic resubscription. This particular pull request isn't related to this issue. Can you subscribe to issue #85 so that you don't miss any progress on this?

[TotalKrill]

I tried wrapping my head around this some. The problem is that the eventloop handles publish requests, and that if we are handling other events during this time, the publish might stall since the other events from the broker might ( such as recieved mqtt messages ) might be in the way. as well as that client generated pings and events can get lost. And all these problems are due to the fact that the client is overloaded?

Could we not return errors when trying to publish to an overloaded eventloop, or send an error notification in the case when a ping failed to send due to overloading the eventloop. Or is this impossible due to problem with blocking the eventloop?

[marcotuna]

Any updates on this subject? I am having troubles when needing to resubscribe when broker crashes.

[tekjar]

@marcotuna I've implemented reconnection events in the master branch. But I'll have to add automatic resubscription to eventloop (of course based on user configuration).

You can use reconnection events to resubscribe manually but the subscription will only happen after the publishes in the queue are transmitted by the eventloop

[tekjar]

@flxo Reconnection notifications and notification drop handling are part of master now. Please feel free to comment on the issue if you don't feel the current behavior solves your usecase

[tekjar]

@marcotuna Can you please head issue #85. I'll expose resubscription option today