From: Zebediah Figura <zfigura@codeweavers.com>
Subject: Re: [PATCH] httpapi: Stub HttpSetUrlGroupProperty(HttpServerTimeoutsProperty property)
Message-Id: <760d7be4-39e4-1021-7329-7aac5cc6e78d@codeweavers.com>
Date: Mon, 14 Sep 2020 11:38:23 -0500
In-Reply-To: <20200913021150.2201112-1-maxwellc@gmail.com>
References: <20200913021150.2201112-1-maxwellc@gmail.com>

Hello Chris, thanks for the patch! I have some comments inlined.

On 9/12/20 9:11 PM, Chris Maxwell wrote:
> Accept HttpServerTimeoutsProperty as valid, but WARN ignoring it and report success to caller.
> 
> This solves a problem in a particular application that fails if a Not Implemented
> error is returned:
> 
> Application performs DotNet calls
> 
>      listener = new HttpListener();
>      listener.TimeoutManager.IdleConnection = TIMEOUT_IDLE;
> 
> which goes through a number of levels of DotNet 4.6.1 to end up calling
> HttpSetUrlGroupProperty with the HttpServerTimeoutsProperty
> inducing "Not Implemented" error.
> 
> My solution, in the style of LoggingProperty portion of commit 52c6070ea5e47d254253dea6f977746d3588e619?hp=4c5f325333da4562b18af8ea846c780221ff319c, is to
> accept the TimeoutsProperty, WARN ignoring it, and report success.
> I would suggest this as sufficient rather than implementing a whole infrastructure
> for detecting timeout in the request queue and such because to now there has
> apparently not been a need for this to implement full scale Internet webserving
> applications.  The requirements for this particular application is to
> offer a service that can be called back to with the results of an SSO service
> so timeouts will be vanishingly rare.  This gets the application running
> to the next step as it does on Windows without loss of functionality.
> 
> The downside is that attempting to run an application that does actually need
> timeout functionality will appear to work until it hangs, rather than placing
> a pressure point on the Wine Project to implement the contemplated web
> server API infrastructure.
> 
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49819
> Signed-off-by: Chris Maxwell <maxwellc@gmail.com>
> ---
>  dlls/httpapi/httpapi_main.c | 11 +++++++++++
>  include/http.h              | 11 +++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/dlls/httpapi/httpapi_main.c b/dlls/httpapi/httpapi_main.c
> index 6be4d11d35..b31e13e6ce 100644
> --- a/dlls/httpapi/httpapi_main.c
> +++ b/dlls/httpapi/httpapi_main.c
> @@ -676,6 +676,17 @@ ULONG WINAPI HttpSetUrlGroupProperty(HTTP_URL_GROUP_ID id, HTTP_SERVER_PROPERTY
>          case HttpServerLoggingProperty:
>              WARN("Ignoring logging property.\n");
>              return ERROR_SUCCESS;
> +        case HttpServerTimeoutsProperty:
> +        {
> +            const HTTP_TIMEOUT_LIMIT_INFO *info = value;
> +	    
> +            TRACE("Attempt to change ServerTimeoutProperty (flag=%x, body=%u, drain=%u, reqq=%u, idle=%u, headw=%u, minsend=%u\n",
> +                  info->Flags, info->EntityBody, info->DrainEntityBody, info->RequestQueue,
> +                  info->IdleConnection, info->HeaderWait, info->MinSendRate);

This yields a warning, because "Flags" is not an integer value but a
structure.

> +            

Please avoid trailing whitespace, here and elsewhere.

> +            WARN("Ignoring timeout property.\n");
> +            return ERROR_SUCCESS;
> +        }

Given that HttpServerTimeoutsProperty actually affects the behaviour of
the server in visible ways, I think using FIXME instead might be a good
idea.

>          default:
>              FIXME("Unhandled property %u.\n", property);
>              return ERROR_CALL_NOT_IMPLEMENTED;
> diff --git a/include/http.h b/include/http.h
> index 3ab57f70bf..fa3d4d02df 100644
> --- a/include/http.h
> +++ b/include/http.h
> @@ -429,6 +429,17 @@ typedef struct _HTTP_BINDING_INFO
>      HANDLE RequestQueueHandle;
>  } HTTP_BINDING_INFO, *PHTTP_BINDING_INFO;
>  
> +typedef struct _HTTP_TIMEOUT_LIMIT_INFO
> +{
> +    HTTP_PROPERTY_FLAGS Flags;
> +    USHORT  EntityBody;
> +    USHORT  DrainEntityBody;
> +    USHORT  RequestQueue;
> +    USHORT  IdleConnection;
> +    USHORT  HeaderWait;
> +    USHORT  MinSendRate;

This structure has inconsistent spacing.

> +} HTTP_TIMEOUT_LIMIT_INFO, *PHTTP_TIMEOUT_LIMIT_INFO;
> +  
>  typedef enum _HTTP_QOS_SETTING_TYPE
>  {
>      HttpQosSettingTypeBandwidth,
> 


-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEENTo75Twe9rbPART3DZ01igeheEAFAl9fnIAACgkQDZ01igeh
eEAULAgAiECcgNnbViORdsF8zbM2Dwag2sdv54WhJduINhpio4GV3lD2QhcNfo6W
3WDop1Ogav1bLflAsP3OpM8dZUjiWtqY1hW85gBeSkukLBLHq+0bxW3+F0NGElPu
Upt5FCHzpECGQkKljTePKDzgiJosPiY5Nvo0Rwx2PwZj4XT/HAT2y1d0UdqS1tqU
xTFaUd8vWhdDJ2bClVWZgwwwuCAAwemw3vsUsINmVbIz/hXY6EDiYxUxuuxsnB7y
3HriwFk4ehKiHBS6BuAd/8ifurJRbAnDd119iQaDfJXbcjkxvkbYJKOcrd3IwaxR
n6uiSgtXrSLtKSjPruEr4+cKawBlmw==
=KY/u
-----END PGP SIGNATURE-----