https://redmine.named-data.net/https://redmine.named-data.net/favicon.ico?14759811232017-03-28T16:00:03ZNDN project issue tracking systemndn-cxx - Feature #4025: NetworkMonitor: empty impl on unsupported platformhttps://redmine.named-data.net/issues/4025?journal_id=187182017-03-28T16:00:03ZJunxiao Shi
<ul><li><strong>Blocked by</strong> <i><a class="issue tracker-2 status-5 priority-2 priority-default closed" href="/issues/3353">Feature #3353</a>: NetworkMonitor: emit fine-grained signals when the state of a network interface changes</i> added</li></ul> ndn-cxx - Feature #4025: NetworkMonitor: empty impl on unsupported platformhttps://redmine.named-data.net/issues/4025?journal_id=187202017-03-28T16:05:30ZDavide Pesavento
<ul></ul><p>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.</p>
ndn-cxx - Feature #4025: NetworkMonitor: empty impl on unsupported platformhttps://redmine.named-data.net/issues/4025?journal_id=187272017-03-29T07:14:19ZJunxiao Shi
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li><li><strong>Assignee</strong> set to <i>Junxiao Shi</i></li><li><strong>Estimated time</strong> set to <i>3.00 h</i></li></ul> ndn-cxx - Feature #4025: NetworkMonitor: empty impl on unsupported platformhttps://redmine.named-data.net/issues/4025?journal_id=187282017-03-29T07:48:51ZJunxiao Shi
<ul><li><strong>Related to</strong> <i><a class="issue tracker-2 status-5 priority-2 priority-default closed" href="/issues/4024">Feature #4024</a>: NetworkMonitor: stub implementation</i> added</li></ul> ndn-cxx - Feature #4025: NetworkMonitor: empty impl on unsupported platformhttps://redmine.named-data.net/issues/4025?journal_id=187302017-03-29T07:58:25ZJunxiao Shi
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Code review</i></li><li><strong>% Done</strong> changed from <i>0</i> to <i>100</i></li></ul><p><a href="https://gerrit.named-data.net/3803">https://gerrit.named-data.net/3803</a> changes throwing to <code>network-monitor-impl-noop.hpp</code> which does nothing.</p>
<p><code>NetworkMonitor::getCapabilities()</code> function reports what functions and signals are supported on current platform. It can even indicate the lack of MTU change signal on macOS.<br>
It is intentionally designed as an instance method rather than a static method, because <a class="issue tracker-2 status-5 priority-2 priority-default closed" title="Feature: NetworkMonitor: stub implementation (Closed)" href="https://redmine.named-data.net/issues/4024">#4024</a> would need to accept <code>Impl*</code> as a constructor parameter in order to accommodate mock impl, after which having a static <code>NetworkMonitor::getCapabilities()</code> function would be infeasible.</p>
ndn-cxx - Feature #4025: NetworkMonitor: empty impl on unsupported platformhttps://redmine.named-data.net/issues/4025?journal_id=187312017-03-29T08:24:19ZDavide Pesavento
<ul></ul><p>Junxiao Shi wrote:</p>
<blockquote>
<p>It is intentionally designed as an instance method rather than a static method, because <a class="issue tracker-2 status-5 priority-2 priority-default closed" title="Feature: NetworkMonitor: stub implementation (Closed)" href="https://redmine.named-data.net/issues/4024">#4024</a> would need to accept <code>Impl*</code> as a constructor parameter in order to accommodate mock impl, after which having a static <code>NetworkMonitor::getCapabilities()</code> function would be infeasible.</p>
</blockquote>
<p>Can you elaborate on this requirement? I'm not sure I'm following.</p>
ndn-cxx - Feature #4025: NetworkMonitor: empty impl on unsupported platformhttps://redmine.named-data.net/issues/4025?journal_id=187372017-03-29T09:39:40ZJunxiao Shi
<ul></ul><p>As indicated in <a class="issue tracker-2 status-5 priority-2 priority-default closed" title="Feature: NetworkMonitor: stub implementation (Closed)" href="https://redmine.named-data.net/issues/4024#note-4">#4024-4</a>, <code>NetworkMonitor</code> constructor takes <code>unique_ptr<ImplBase></code>. Therefore, the capabilities of a <code>NetworkMonitor</code> can differ per instance. This allows <code>getCapabilities()</code> return value to be mocked.</p>
ndn-cxx - Feature #4025: NetworkMonitor: empty impl on unsupported platformhttps://redmine.named-data.net/issues/4025?journal_id=187462017-03-29T20:30:53ZDavide Pesavento
<ul></ul><p>The problem with requiring an instance on which to call <code>getCapabilities()</code> is that, once the constructor of <code>NetworkMonitor</code> is called, things already start happening (sockets are created, async work is scheduled, etc...), so it might already be too late.</p>
ndn-cxx - Feature #4025: NetworkMonitor: empty impl on unsupported platformhttps://redmine.named-data.net/issues/4025?journal_id=187482017-03-29T21:27:48ZJunxiao Shi
<ul></ul><p>Reply to note-8:</p>
<p>How is that "too late"? The caller is supposed to connect all signals as soon as <code>NetworkMonitor</code> is constructed. <code>getCapabilities()</code> is purely informational.</p>
ndn-cxx - Feature #4025: NetworkMonitor: empty impl on unsupported platformhttps://redmine.named-data.net/issues/4025?journal_id=188122017-04-03T17:13:06ZDavide Pesavento
<ul></ul><p>Junxiao Shi wrote:</p>
<blockquote>
<p>How is that "too late"? The caller is supposed to connect all signals as soon as <code>NetworkMonitor</code> is constructed. <code>getCapabilities()</code> is purely informational.</p>
</blockquote>
<p>With "too late" I mean that a lot of work has already happened in the constructor of <code>NetworkMonitor</code>, 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.</p>
<p>The idiomatic way, as Alex also said in code review, is using a static member function. Do we really need to mock <code>getCapabilities()</code> itself?</p>
ndn-cxx - Feature #4025: NetworkMonitor: empty impl on unsupported platformhttps://redmine.named-data.net/issues/4025?journal_id=188132017-04-03T17:20:33ZJunxiao Shi
<ul></ul><blockquote>
<p>it's wasteful to construct an instance just to query its capabilities</p>
</blockquote>
<p>As commented in <a href="https://gerrit.named-data.net/#/c/3811/1/src/util/face-uri.cpp@478">https://gerrit.named-data.net/#/c/3811/1/src/util/face-uri.cpp@478</a>, each application is expected to have only one instance of <code>NetworkMonitor</code>, so there would be no waste.</p>
<blockquote>
<p>Do we really need to mock <code>getCapabilities()</code> itself?</p>
</blockquote>
<p>Yes, it's necessary to mock everything including <code>getCapabilities</code>. 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, <code>getCapabilities</code> must be mocked.</p>
ndn-cxx - Feature #4025: NetworkMonitor: empty impl on unsupported platformhttps://redmine.named-data.net/issues/4025?journal_id=188182017-04-03T17:36:19ZDavide Pesavento
<ul></ul><p>Junxiao Shi wrote:</p>
<blockquote>
<blockquote>
<p>it's wasteful to construct an instance just to query its capabilities</p>
</blockquote>
<p>As commented in <a href="https://gerrit.named-data.net/#/c/3811/1/src/util/face-uri.cpp@478">https://gerrit.named-data.net/#/c/3811/1/src/util/face-uri.cpp@478</a>, each application is expected to have only one instance of <code>NetworkMonitor</code>, so there would be no waste.</p>
</blockquote>
<p>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.</p>
ndn-cxx - Feature #4025: NetworkMonitor: empty impl on unsupported platformhttps://redmine.named-data.net/issues/4025?journal_id=193602017-05-30T14:41:50ZDavide Pesavento
<ul><li><strong>Status</strong> changed from <i>Code review</i> to <i>Closed</i></li></ul>