lumeohq / onvif-rs

A native Rust ONVIF client library.
MIT License
115 stars 61 forks source link

Improve discovery timings #83

Closed DmitrySamoylov closed 3 years ago

DmitrySamoylov commented 3 years ago

Story details: https://app.shortcut.com/lumeo/story/4028

shortcut-integration[bot] commented 3 years ago

This pull request has been linked to Clubhouse Story #4028: Lumeod fails to discover all cameras when the network has a large number of cameras (>18).

jplatte commented 3 years ago

Independently of the flat_map thing, I think the pattern of iter.map(/* async -> Option */).collect::<FuturesUnordered<_>>().filter_map(ready) is easier to understand when written as futures::stream::iter(iter).filter_map(/* async -> Option */).collect::<FuturesUnordered<_>>(). Or am I missing sth. and that's not the same / doesn't work?

DmitrySamoylov commented 3 years ago

Execution order is a bit different. FuturesUnordered should be collected from an iterator over futures, not from a stream. So it collects futures into something without even polling them.

futures::stream::iter(iter).filter_map... instead would try to execute futures in order as they appear in originating iterator.

jplatte commented 3 years ago

Right. Well, when reading it didn't really stand out to me that only a single item from the FuturesUnordered is used. Maybe we can move this logic out into a separate function similar to future::select_all (maybe called select_unordered or similar)?

DmitrySamoylov commented 3 years ago

You mean that next() is not sufficiently highlighted? When looking at it and at return type Option<Device> it should be clear that only one element is used.

BTW reading docs of select_all I think we can use it instead of FuturesUnordered :thinking:

jplatte commented 3 years ago

Let's use select_all then? I was assuming that we're using FuturesUnordered for good reason (i.e. very many futures).

DmitrySamoylov commented 3 years ago

Switched to select_all but to my taste FuturesUnordered looks cleaner as it's API works better with chained calls.

DmitrySamoylov commented 3 years ago

It was worth trying :smiley: