[PATCH 2/8] wsdapi/tests: Add test for duplicate message filtering.

[Owen Rudge]

Signed-off-by: Owen Rudge 
---
  dlls/wsdapi/tests/discovery.c | 134 ++++++++++++++++++++++++++++++++--
  1 file changed, 129 insertions(+), 5 deletions(-)

[Huw Davies]

On Thu, Sep 13, 2018 at 09:44:34AM +0100, Owen Rudge wrote:
> Signed-off-by: Owen Rudge
>
> dlls/wsdapi/tests/discovery.c | 134 ++++++++++++++++++++++++++++++++--
>  1 file changed, 129 insertions(+), 5 deletions(-)
> diff --git a/dlls/wsdapi/tests/discovery.c b/dlls/wsdapi/tests/discovery.c
> index f33f469a03..9e1cbf99e2 100644
> --- a/dlls/wsdapi/tests/discovery.c
> +++ b/dlls/wsdapi/tests/discovery.c
> @@ -72,6 +72,7 @@ static const WCHAR name_cider[] = { 'C','i','d','e','r', 0 };
>
> static HANDLE probe_event = NULL;
>  static UUID probe_message_id;
> +static int probe_message_received_count;
>
> static IWSDiscoveryPublisher *publisher_instance = NULL;
>
> @@ -790,6 +791,37 @@ after_matchprobe_test:
>      return S_OK;
>  }
>
> +static HRESULT WINAPI IWSDiscoveryPublisherNotify_MsgCount_Impl_ProbeHandler(IWSDiscoveryPublisherNotify *This,
> - const WSD_SOAP_MESSAGE *pSoap, IWSDMessageParameters *pMessageParameters)
>
> +{
> - trace("IWSDiscoveryPublisherNotify_MsgCount_Impl_ProbeHandler called (%p, %p, %p)\n", This, pSoap, pMessageParameters);
> -
> - ok(pSoap != NULL, "pSoap == NULL\n");
> -
> - if (pSoap != NULL)
> - {
> -
>    ok(pSoap->Header.MessageID != NULL, "pSoap->Header.MessageID == NULL\n");
> -
> -
>    /* Ensure the message ID is at least 9 characters long (to skip past the 'urn:uuid:' prefix) */
> -
>    if ((pSoap->Header.MessageID != NULL) && (lstrlenW(pSoap->Header.MessageID) > 9))
> -
>    {
> -
>        UUID uuid;
> -
>        RPC_STATUS ret = UuidFromStringW((LPWSTR)pSoap->Header.MessageID + 9, &uuid);
> -
> -
>        trace("Received message with UUID '%s' (expected UUID '%s')\n", wine_dbgstr_guid(&uuid),
> -
>            wine_dbgstr_guid(&probe_message_id));
> -
> -
>        /* Check if we've either received a message without a UUID, or the UUID isn't the one we sent. If so,
> -
>           ignore it and wait for another message. */
> -
>        if ((ret != RPC_S_OK) || (UuidEqual(&uuid, &probe_message_id, &ret) == FALSE)) return S_OK;
> -
> -
>        InterlockedIncrement(&probe_message_received_count);
> -
>    }
> - }
> -
> - return S_OK;
>
> +}
> -
>
> static HRESULT WINAPI IWSDiscoveryPublisherNotifyImpl_ResolveHandler(IWSDiscoveryPublisherNotify *This, const WSD_SOAP_MESSAGE *pSoap, IWSDMessageParameters *pMessageParameters)
>  {
>      trace("IWSDiscoveryPublisherNotifyImpl_ResolveHandler called (%p, %p, %p)\n", This, pSoap, pMessageParameters);
> @@ -805,7 +837,17 @@ static const IWSDiscoveryPublisherNotifyVtbl publisherNotify_vtbl =
>      IWSDiscoveryPublisherNotifyImpl_ResolveHandler
>  };
>
> -static BOOL create_discovery_publisher_notify(IWSDiscoveryPublisherNotify **publisherNotify)
> +static const IWSDiscoveryPublisherNotifyVtbl publisherNotify_MsgCount_vtbl =
> +{
> - IWSDiscoveryPublisherNotifyImpl_QueryInterface,
> - IWSDiscoveryPublisherNotifyImpl_AddRef,
> - IWSDiscoveryPublisherNotifyImpl_Release,
> - IWSDiscoveryPublisherNotify_MsgCount_Impl_ProbeHandler,
> - IWSDiscoveryPublisherNotifyImpl_ResolveHandler
>
> +};
> -
>
> +static BOOL create_discovery_publisher_notify(IWSDiscoveryPublisherNotify **publisherNotify,
> - const IWSDiscoveryPublisherNotifyVtbl *vtbl)
>
> {
>      IWSDiscoveryPublisherNotifyImpl *obj;
>
> @@ -819,7 +861,7 @@ static BOOL create_discovery_publisher_notify(IWSDiscoveryPublisherNotify **publ
>          return FALSE;
>      }
> - obj->IWSDiscoveryPublisherNotify_iface.lpVtbl = &publisherNotify_vtbl;
> -
>
> obj->IWSDiscoveryPublisherNotify_iface.lpVtbl = vtbl;
> obj->ref = 1;
>
> *publisherNotify = &obj->IWSDiscoveryPublisherNotify_iface;
>
> @@ -959,8 +1001,8 @@ static void Publish_tests(void)
>      ok(rc == STG_E_INVALIDFUNCTION, "IWSDiscoveryPublisher_SetAddressFamily(WSDAPI_ADDRESSFAMILY_IPV6) returned unexpected result: %08x\n", rc);
>  /* Create notification sinks */
> - ok(create_discovery_publisher_notify(&sink1) == TRUE, "create_discovery_publisher_notify failed\n");
> - ok(create_discovery_publisher_notify(&sink2) == TRUE, "create_discovery_publisher_notify failed\n");
> -
>
> ok(create_discovery_publisher_notify(&sink1, &publisherNotify_vtbl) == TRUE, "create_discovery_publisher_notify failed\n");
> -
>
> ok(create_discovery_publisher_notify(&sink2, &publisherNotify_vtbl) == TRUE, "create_discovery_publisher_notify failed\n");
>
> /* Get underlying implementation so we can check the ref count */
> sink1Impl = impl_from_IWSDiscoveryPublisherNotify(sink1);
>
> @@ -1200,7 +1242,7 @@ static void UnPublish_tests(void)
>      ok(rc == S_OK, "IWSDiscoveryPublisher_SetAddressFamily(WSDAPI_ADDRESSFAMILY_IPV4) failed: %08x\n", rc);
>  /* Create notification sink */
> - ok(create_discovery_publisher_notify(&sink1) == TRUE, "create_discovery_publisher_notify failed\n");
> - ok(create_discovery_publisher_notify(&sink1, &publisherNotify_vtbl) == TRUE, "create_discovery_publisher_notify failed\n");
> rc = IWSDiscoveryPublisher_RegisterNotificationSink(publisher, sink1);
> ok(rc == S_OK, "IWSDiscoveryPublisher_RegisterNotificationSink failed: %08x\n", rc);
>
> @@ -1307,6 +1349,87 @@ after_unpublish_test:
>      WSACleanup();
>  }
>
> +static void Probe_DuplicateMessageFiltering_tests(void)
> +{
> - IWSDiscoveryPublisher *publisher = NULL;
> - IWSDiscoveryPublisherNotify *sink1 = NULL;
> - LPWSTR publisherIdW = NULL, sequenceIdW = NULL;
> - WSADATA wsaData;
> - int ret;
> - HRESULT rc;
> - ULONG ref;
> - unsigned char *probe_uuid_str;
> -
> - rc = WSDCreateDiscoveryPublisher(NULL, &publisher);
> - ok(rc == S_OK, "WSDCreateDiscoveryPublisher(NULL, &publisher) failed: %08x\n", rc);
> - ok(publisher != NULL, "WSDCreateDiscoveryPublisher(NULL, &publisher) failed: publisher == NULL\n");
> -
> - publisher_instance = publisher;
> -
> - rc = IWSDiscoveryPublisher_SetAddressFamily(publisher, WSDAPI_ADDRESSFAMILY_IPV4);
> - ok(rc == S_OK, "IWSDiscoveryPublisher_SetAddressFamily(WSDAPI_ADDRESSFAMILY_IPV4) failed: %08x\n", rc);
> -
> - /* Create notification sink */
> - ok(create_discovery_publisher_notify(&sink1, &publisherNotify_MsgCount_vtbl) == TRUE,
> -
>    "create_discovery_publisher_notify failed\n");
> -
> - /* Register notification sinks */
> - rc = IWSDiscoveryPublisher_RegisterNotificationSink(publisher, sink1);
> - ok(rc == S_OK, "IWSDiscoveryPublisher_RegisterNotificationSink failed: %08x\n", rc);
> -
> - /* Set up network listener */
> - publisherIdW = utf8_to_wide(publisherId);
> - if (publisherIdW == NULL) goto after_test;
> -
> - sequenceIdW = utf8_to_wide(sequenceId);
> - if (sequenceIdW == NULL) goto after_test;
> -
> - ret = WSAStartup(MAKEWORD(2, 2), &wsaData);
> - ok(ret == 0, "WSAStartup failed (ret = %d)\n", ret);
> -
> - /* Publish the service */
> - rc = IWSDiscoveryPublisher_Publish(publisher, publisherIdW, 1, 1, 1, sequenceIdW, NULL, NULL, NULL);
> - ok(rc == S_OK, "Publish failed: %08x\n", rc);
> -
> - heap_free(publisherIdW);
> - heap_free(sequenceIdW);
> -
> - /* Test the receiving of a probe message */
> - UuidCreate(&probe_message_id);
> - UuidToStringA(&probe_message_id, &probe_uuid_str);
> -
> - ok(probe_uuid_str != NULL, "Failed to create UUID for probe message\n");
> -
> - if (probe_uuid_str != NULL)
> - {
> -
>    char probe_message[sizeof(testProbeMessage) + 50];
> -
>    int i;
> -
> -
>    sprintf(probe_message, testProbeMessage, probe_uuid_str);
> -
> -
>    for (i = 0; i < 3; i++)
> -
>    {
> -
>        ok(send_udp_multicast_of_type(probe_message, strlen(probe_message), AF_INET) == TRUE, "Sending Probe message failed\n");
> -
>    }
> -
> -
>    // Wait for the messages to be received

C++ comment.
> -
>    Sleep(1000);

Can't we poll for this?  The tests are starting to take quite a long
time to complete, and this doesn't help.
> -
> -
>    todo_wine ok(probe_message_received_count == 1, "Received %d probe messages\n", probe_message_received_count);
> -
>    RpcStringFreeA(&probe_uuid_str);
> - }
> -
>
> +after_test:
> -
> - ref = IWSDiscoveryPublisher_Release(publisher);
> - ok(ref == 0, "IWSDiscoveryPublisher_Release() has %d references, should have 0\n", ref);
> -
> - /* Release the sinks */
> - IWSDiscoveryPublisherNotify_Release(sink1);
> -
> - WSACleanup();
>
> +}
> -
>
> enum firewall_op
>  {
>      APP_ADD,
> @@ -1453,6 +1576,7 @@ START_TEST(discovery)
>      CreateDiscoveryPublisher_XMLContext_tests();
>      Publish_tests();
>      UnPublish_tests();
> -
>
> Probe_DuplicateMessageFiltering_tests();
>
> CoUninitialize();
> if (firewall_enabled) set_firewall(APP_REMOVE);
>

[Huw Davies]

On Thu, Sep 13, 2018 at 11:05:06AM +0100, Huw Davies wrote:
> On Thu, Sep 13, 2018 at 09:44:34AM +0100, Owen Rudge wrote:
> > -
> >    Sleep(1000);
>
> Can't we poll for this?  The tests are starting to take quite a long
> time to complete, and this doesn't help.

Besides which it's a race.

Huw.

[Owen Rudge]

On 13/09/2018 11:05, Huw Davies wrote:
> > +        Sleep(1000);
>
> Can't we poll for this?  The tests are starting to take quite a long
> time to complete, and this doesn't help.

I'm not sure there is a very good way of avoiding it. We're effectively 
waiting for an extra 2 messages *not* to be received. We can easily test 
when the first message has been received and processed of course, but 
there's no way of identifying when the subsequent messages have been 
received. WSD does not expose any kind of "duplicate message" 
notification that we could listen for.

What I could do is reduce the delay down from 1000ms to, say, 250ms. The 
UDP messages are sent immediately rather than with a variable delay (as 
per WSD itself), so they should be processed and dispatched quickly. In 
my tests this works OK, but of course it could conceivably be more 
vulnerable to sporadic failures on slow or very busy systems.

I can also try to look at any other delays that are already present in 
the tests and see if they can be reduced/eliminated once this series is 
complete.

Thanks,

Owen

[Huw Davies]

On Mon, Sep 17, 2018 at 09:24:48PM +0100, Owen Rudge wrote:
> On 13/09/2018 11:05, Huw Davies wrote:
> > > +        Sleep(1000);
> >
> > Can't we poll for this?  The tests are starting to take quite a long
> > time to complete, and this doesn't help.
>
> I'm not sure there is a very good way of avoiding it. We're effectively
> waiting for an extra 2 messages *not* to be received. We can easily test
> when the first message has been received and processed of course, but
> there's no way of identifying when the subsequent messages have been
> received. WSD does not expose any kind of "duplicate message" notification
> that we could listen for.
>
> What I could do is reduce the delay down from 1000ms to, say, 250ms. The UDP
> messages are sent immediately rather than with a variable delay (as per WSD
> itself), so they should be processed and dispatched quickly. In my tests
> this works OK, but of course it could conceivably be more vulnerable to
> sporadic failures on slow or very busy systems.

How much do we lose by not testing this feature?  If there's no reliable
way to do it then it may be better to simply not.
> I can also try to look at any other delays that are already present in the
> tests and see if they can be reduced/eliminated once this series is
> complete.

That would be good in any case.

Huw.

[Owen Rudge]

On 18/09/2018 08:35, Huw Davies wrote:
> How much do we lose by not testing this feature?  If there's no reliable
> way to do it then it may be better to simply not.

Not a massive amount - if we're accepting duplicate messages, it can 
occasionally manifest in other ways (as we saw with the tests running on 
your machine). I can drop this patch if that seems the best solution.

Thanks,

Owen