Re: TSFetchUrl support for dechunking in HTTP/1.1 - new API TSFetchFlagSet, change return for TSFetchUrl

[Sudheer Vinukonda <su...@yahoo.com.INVALID>]

Also, In addition to a new API TSFetchFlagSet to set the fetch flags,
also need to change the return type for TSFetchUrl() to return the FetchSM context.

TSFetchSM
TSFetchUrl(const char *headers, int request_len, sockaddr const *ip, TSCont contp, TSFetchWakeUpOptions callback_options,
           TSFetchEvent events)

> +void
> +TSFetchFlagSet(TSFetchSM fetch_sm, int flags)



> On Mar 6, 2020, at 8:40 AM, Sudheer Vinukonda <su...@yahoo.com> wrote:
> 
> 
> Sorry, I just realized that TSFetchUrl() can be called directly without calling TSFetchCreate(). So, to preserve backward compatibility and to be able to set the fetch flags, I'm going to add a new `TSFetchFlagSet` API as below. 
> 
> The API accepts a previously created FetchSM context and the flags to be set on it.
> 
> 
> +void
> +TSFetchFlagSet(TSFetchSM fetch_sm, int flags)
> +{
> +  sdk_assert(sdk_sanity_check_fetch_sm(fetch_sm) == TS_SUCCESS);
> +  (reinterpret_cast<FetchSM *>(fetch_sm))->set_fetch_flags(flags);
> +}
> +
> 
> 
> Since this is a new API, I'll wait for a few more days for the review.
> 
> 
> 
> 
> 
> On Friday, March 6, 2020, 08:03:03 AM PST, Sudheer Vinukonda <su...@yahoo.com> wrote:
> 
> 
> I'm going to call this in and make the change to the TSFetchUrl() API to remove setting `TS_FETCH_FLAGS_DECHUNK` flag by default.
> 
> After discussing with Leif over slack, it's worth explicitly clarifying that plugins that do want to still have `TS_FETCH_FLAGS_DECHUNK` set can do so by simply passing the flag during `TSFetchCreate()` explicitly (that is exactly what we are going to do in our plugins).
> 
> 
> -- Sudheer
> 
> 
> 
> 
> 
> On Wednesday, March 4, 2020, 10:06:00 AM PST, Sudheer Vinukonda <su...@yahoo.com.invalid> wrote:
> 
> 
> We use TSFetchUrl() API in a bunch of our use cases to trigger sideways calls. The API by default sets `TS_FETCH_FLAGS_DECHUNK`,which implies the response returned is dechunked. There's another flag called 'TS_FETCH_FLAGS_STREAM' in FechSM, which is NOT set by default. 
> This makes TSFetchUrl API a bit conflating and contradictory since dechunking is only done when 'TS_FETCH_FLAGS_STREAM' is set ignoring the `TS_FETCH_FLAGS_DECHUNK`.
> Our plugins can not handle chunked responses on their own and we don't want TS_FETCH_FLAGS_STREAM. We just want the entire response (headers + body) to be returned together set by the callback_options AFTER_BODY (similar to how esi plugin works).
> The side-effect of all of this is that we can not use HTTP/1.1 to the Origins return chunked responses and the contradictory flags above mean that our plugins end up getting chunked body (with chunk sizes, trailers etc). 
> I'd like to change the default TSFetchUrl() API mode to not set `TS_FETCH_FLAGS_DECHUNK` anymore and let callers ask explicitly if they want dechunked body similar to how  'TS_FETCH_FLAGS_STREAM'  flag works. 
> The resultant behavior would be that a plugin would get chunked body only when it does not set ` 'TS_FETCH_FLAGS_STREAM' ` and `TS_FETCH_FLAGS_DECHUNK` explicitly. If it does either, it'd get dechunked body (streamed in the first case and non-streamed in the latter).
> This will make the flags consistent and preserves the backward compatibility that esi plugin needs as it does not set `TS_FETCH_FLAGS_STREAM` (and `TS_FETCH_FLAGS_DECHUNK`) explicitly. We will update our plugins to explicitly ask for `TS_FETCH_FLAGS_DECHUNK`.
> Let me know if there are any concerns/suggestions.