Project

General

Profile

Feature #4025

NetworkMonitor: empty impl on unsupported platform

Added by Junxiao Shi over 3 years ago. Updated over 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Utils
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
3.00 h

Description

When NetworkMonitor is instantiated on an unsupported platform, instead of throwing an exception in constructor, a no-op Impl should be provided.
In this case, NetworkMonitor behaves as if there is no network interface on the system, and does not emit any signals.


Related issues

Related to ndn-cxx - Feature #4024: NetworkMonitor: stub implementationClosedJunxiao Shi

Actions
Blocked by ndn-cxx - Feature #3353: NetworkMonitor: emit fine-grained signals when the state of a network interface changesClosedDavide Pesavento

Actions
#1

Updated by Junxiao Shi over 3 years ago

  • Blocked by Feature #3353: NetworkMonitor: emit fine-grained signals when the state of a network interface changes added
#2

Updated by Davide Pesavento over 3 years ago

This is not enough. There needs to be a way to let the library consumer know that monitoring is supported/not supported on the current platform, at either compile- or run-time.

#3

Updated by Junxiao Shi over 3 years ago

  • Status changed from New to In Progress
  • Assignee set to Junxiao Shi
  • Estimated time set to 3.00 h
#4

Updated by Junxiao Shi over 3 years ago

  • Related to Feature #4024: NetworkMonitor: stub implementation added
#5

Updated by Junxiao Shi over 3 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 0 to 100

https://gerrit.named-data.net/3803 changes throwing to network-monitor-impl-noop.hpp which does nothing.

NetworkMonitor::getCapabilities() function reports what functions and signals are supported on current platform. It can even indicate the lack of MTU change signal on macOS.
It is intentionally designed as an instance method rather than a static method, because #4024 would need to accept Impl* as a constructor parameter in order to accommodate mock impl, after which having a static NetworkMonitor::getCapabilities() function would be infeasible.

#6

Updated by Davide Pesavento over 3 years ago

Junxiao Shi wrote:

It is intentionally designed as an instance method rather than a static method, because #4024 would need to accept Impl* as a constructor parameter in order to accommodate mock impl, after which having a static NetworkMonitor::getCapabilities() function would be infeasible.

Can you elaborate on this requirement? I'm not sure I'm following.

#7

Updated by Junxiao Shi over 3 years ago

As indicated in #4024-4, NetworkMonitor constructor takes unique_ptr<ImplBase>. Therefore, the capabilities of a NetworkMonitor can differ per instance. This allows getCapabilities() return value to be mocked.

#8

Updated by Davide Pesavento over 3 years ago

The problem with requiring an instance on which to call getCapabilities() is that, once the constructor of NetworkMonitor is called, things already start happening (sockets are created, async work is scheduled, etc...), so it might already be too late.

#9

Updated by Junxiao Shi over 3 years ago

Reply to note-8:

How is that "too late"? The caller is supposed to connect all signals as soon as NetworkMonitor is constructed. getCapabilities() is purely informational.

#10

Updated by Davide Pesavento over 3 years ago

Junxiao Shi wrote:

How is that "too late"? The caller is supposed to connect all signals as soon as NetworkMonitor is constructed. getCapabilities() is purely informational.

With "too late" I mean that a lot of work has already happened in the constructor of NetworkMonitor, so it's wasteful to construct an instance just to query its capabilities, which don't actually depend on the instance itself but on the platform on which NetworkMonitor is running.

The idiomatic way, as Alex also said in code review, is using a static member function. Do we really need to mock getCapabilities() itself?

#11

Updated by Junxiao Shi over 3 years ago

it's wasteful to construct an instance just to query its capabilities

As commented in https://gerrit.named-data.net/#/c/3811/1/src/util/face-uri.cpp@478, each application is expected to have only one instance of NetworkMonitor, so there would be no waste.

Do we really need to mock getCapabilities() itself?

Yes, it's necessary to mock everything including getCapabilities. For example, the calling app may launch a polling session for MTU changes, if MTU signals are not supported but needed by the app. To test this logic, getCapabilities must be mocked.

#12

Updated by Davide Pesavento over 3 years ago

Junxiao Shi wrote:

it's wasteful to construct an instance just to query its capabilities

As commented in https://gerrit.named-data.net/#/c/3811/1/src/util/face-uri.cpp@478, each application is expected to have only one instance of NetworkMonitor, so there would be no waste.

The waste would happen when an app realizes that it cannot use the NetworkMonitor it just constructed because it doesn't provide the required functionality, and therefore destructs it because the app has an internal fallback or can work without it (e.g. it's an optional feature). Anyway, this might be a very minor concern that we can ignore if there's no other solution.

#13

Updated by Davide Pesavento over 3 years ago

  • Status changed from Code review to Closed

Also available in: Atom PDF