Bug 182979 – [Curl] Split DNS cache expiration and connection timeout setting.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Basuke Suzuki]

It was mis-implemented and was named ambiguous name 'enableTimeout'. Implement each feature correctly.

[Basuke Suzuki]

Created attachment 334458 [details]
Patch

[Don Olmstead]

Comment on attachment 334458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334458&action=review

> Source/WebCore/platform/network/curl/CurlContext.cpp:69
> +}

Fix alignment

[Basuke Suzuki]

Created attachment 334475 [details]
style fix

[Basuke Suzuki]

(In reply to Don Olmstead from comment #2)
> Comment on attachment 334458 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334458&action=review
> 
> > Source/WebCore/platform/network/curl/CurlContext.cpp:69
> > +}
> 
> Fix alignment

Thanks.

[Per Arne Vollan]

Comment on attachment 334475 [details]
style fix

View in context: https://bugs.webkit.org/attachment.cgi?id=334475&action=review

> Source/WebCore/platform/network/curl/CurlContext.cpp:70
> +    if (const char* str = getenv("WEBKIT_CURL_DNS_CACHE_TIMEOUT")) {
> +        unsigned seconds = 0;
> +        sscanf(str, "%lud", &seconds);
> +
> +        m_dnsCacheTimeoutSeconds = seconds;
> +    }
> +
> +    if (const char* str = getenv("WEBKIT_CURL_CONNECT_TIMEOUT")) {
> +        unsigned seconds = 0;
> +        sscanf(str, "%lud", &seconds);
> +
> +        m_connectTimeoutSeconds = seconds;
> +    }
> +

Perhaps we can do without this, since the default values should be appropriate in most cases?

> Source/WebCore/platform/network/curl/CurlContext.h:141
> +    unsigned m_dnsCacheTimeoutSeconds { 5 * 60 };
> +    unsigned m_connectTimeoutSeconds { 30 };

Would it make sense to use the Seconds class here?

[Basuke Suzuki]

(In reply to Per Arne Vollan from comment #5)
> Comment on attachment 334475 [details]
> style fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334475&action=review
> 
> > Source/WebCore/platform/network/curl/CurlContext.cpp:70
> > +    if (const char* str = getenv("WEBKIT_CURL_DNS_CACHE_TIMEOUT")) {
> > +        unsigned seconds = 0;
> > +        sscanf(str, "%lud", &seconds);
> > +
> > +        m_dnsCacheTimeoutSeconds = seconds;
> > +    }
> > +
> > +    if (const char* str = getenv("WEBKIT_CURL_CONNECT_TIMEOUT")) {
> > +        unsigned seconds = 0;
> > +        sscanf(str, "%lud", &seconds);
> > +
> > +        m_connectTimeoutSeconds = seconds;
> > +    }
> > +
> 
> Perhaps we can do without this, since the default values should be
> appropriate in most cases?

We want these values configurable because our platform sometimes has slightly different requirements.

By the way, are there any default definition for these values? We refer Mozilla's documentation for these, but if WebKit has specific values, we wanna use them.

> 
> > Source/WebCore/platform/network/curl/CurlContext.h:141
> > +    unsigned m_dnsCacheTimeoutSeconds { 5 * 60 };
> > +    unsigned m_connectTimeoutSeconds { 30 };
> 
> Would it make sense to use the Seconds class here?

Oh, great. Another chance to adopt WebKit classes. I will use it.

[Basuke Suzuki]

Created attachment 334699 [details]
Fix to use Seconds class

[Per Arne Vollan]

(In reply to Basuke Suzuki from comment #6)
> (In reply to Per Arne Vollan from comment #5)
> > Comment on attachment 334475 [details]
> > style fix
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=334475&action=review
> > 
> > > Source/WebCore/platform/network/curl/CurlContext.cpp:70
> > > +    if (const char* str = getenv("WEBKIT_CURL_DNS_CACHE_TIMEOUT")) {
> > > +        unsigned seconds = 0;
> > > +        sscanf(str, "%lud", &seconds);
> > > +
> > > +        m_dnsCacheTimeoutSeconds = seconds;
> > > +    }
> > > +
> > > +    if (const char* str = getenv("WEBKIT_CURL_CONNECT_TIMEOUT")) {
> > > +        unsigned seconds = 0;
> > > +        sscanf(str, "%lud", &seconds);
> > > +
> > > +        m_connectTimeoutSeconds = seconds;
> > > +    }
> > > +
> > 
> > Perhaps we can do without this, since the default values should be
> > appropriate in most cases?
> 
> We want these values configurable because our platform sometimes has
> slightly different requirements.
> 

Are there other ways we could make this configurable?

[Basuke Suzuki]

(In reply to Per Arne Vollan from comment #8)
> Are there other ways we could make this configurable?

Using some kind of CreationParameter is our plan when our WebKit2 implementation goes to wincairo. Until then we wanna keep using the old way to stay away from unneeded effort if possible.

[Basuke Suzuki]

(environment variable was used in wincairo before we've start maintaining the port).

[Per Arne Vollan]

Comment on attachment 334699 [details]
Fix to use Seconds class

View in context: https://bugs.webkit.org/attachment.cgi?id=334699&action=review

> Source/WebCore/platform/network/curl/CurlContext.cpp:59
> +        sscanf(str, "%lud", &timeout);

Should we check the return value before assigning?

[Per Arne Vollan]

Comment on attachment 334699 [details]
Fix to use Seconds class

View in context: https://bugs.webkit.org/attachment.cgi?id=334699&action=review

> Source/WebCore/platform/network/curl/CurlContext.cpp:66
> +        sscanf(str, "%lud", &timeout);

Is 'sscanf(str, "%u", &timeout);' sufficient here?

[Basuke Suzuki]

(In reply to Per Arne Vollan from comment #11)
> Comment on attachment 334699 [details]
> Fix to use Seconds class
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334699&action=review
> 
> > Source/WebCore/platform/network/curl/CurlContext.cpp:59
> > +        sscanf(str, "%lud", &timeout);
> 
> Should we check the return value before assigning?

That's safer. I'll add them.

[Basuke Suzuki]

(In reply to Per Arne Vollan from comment #12)
> Comment on attachment 334699 [details]
> Fix to use Seconds class
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334699&action=review
> 
> > Source/WebCore/platform/network/curl/CurlContext.cpp:66
> > +        sscanf(str, "%lud", &timeout);
> 
> Is 'sscanf(str, "%u", &timeout);' sufficient here?


Right. Actually "l" is a bug. I'll fix it.

[Basuke Suzuki]

Created attachment 334768 [details]
FIX

[Per Arne Vollan]

Comment on attachment 334768 [details]
FIX

View in context: https://bugs.webkit.org/attachment.cgi?id=334768&action=review

R=me.

> Source/WebCore/platform/network/curl/CurlContext.cpp:67
> +class EnvironmentVariableReader {
> +public:
> +    template<typename T> std::optional<T> readAs(const char* name)
> +    {
> +        if (const char* valueStr = ::getenv(name)) {
> +            T value;
> +            if (sscanf(valueStr, sscanTemplate<T>(), &value) == 1)
> +                return value;
> +        }
> +
> +        return std::nullopt;
> +    }
> +
> +private:
> +    template<typename T> const char* sscanTemplate()
> +    {
> +        ASSERT_NOT_REACHED();
> +        return nullptr;
> +    }
> +
> +    template<> const char* sscanTemplate<unsigned>() { return "%u"; }
> +};
> +

This class could also be put in a header file for reuse. Perhaps we also could consider using a namespace instead?

> Source/WebCore/platform/network/curl/CurlContext.cpp:483
> +    unsigned value = time >= 0.0 ? static_cast<unsigned>(time) : 0;

For the sake of clarity, I think we should use parentheses here: (time >= 0.0)

[Basuke Suzuki]

(In reply to Per Arne Vollan from comment #16)
> Comment on attachment 334768 [details]
> FIX
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334768&action=review
> 
> R=me.
> 
> > Source/WebCore/platform/network/curl/CurlContext.cpp:67
> > +class EnvironmentVariableReader {
> > +public:
> > +    template<typename T> std::optional<T> readAs(const char* name)
> > +    {
> > +        if (const char* valueStr = ::getenv(name)) {
> > +            T value;
> > +            if (sscanf(valueStr, sscanTemplate<T>(), &value) == 1)
> > +                return value;
> > +        }
> > +
> > +        return std::nullopt;
> > +    }
> > +
> > +private:
> > +    template<typename T> const char* sscanTemplate()
> > +    {
> > +        ASSERT_NOT_REACHED();
> > +        return nullptr;
> > +    }
> > +
> > +    template<> const char* sscanTemplate<unsigned>() { return "%u"; }
> > +};
> > +
> 
> This class could also be put in a header file for reuse. Perhaps we also
> could consider using a namespace instead?

I put this code here in implementation because I shame on using environment variables. Are there any case we gonna use this pattern? We will remove this once we switched to WK2.


> 
> > Source/WebCore/platform/network/curl/CurlContext.cpp:483
> > +    unsigned value = time >= 0.0 ? static_cast<unsigned>(time) : 0;
> 
> For the sake of clarity, I think we should use parentheses here: (time >=
> 0.0)

Or move entire conditional statement inside statice_cast:

unsigned value = static_cast<unsigned>(time >= 0.0 ? time : 0);

Much easier to read. What do you think?

[Per Arne Vollan]

(In reply to Basuke Suzuki from comment #17)
> (In reply to Per Arne Vollan from comment #16)
> > Comment on attachment 334768 [details]
> > FIX
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=334768&action=review
> > 
> > R=me.
> > 
> > > Source/WebCore/platform/network/curl/CurlContext.cpp:67
> > > +class EnvironmentVariableReader {
> > > +public:
> > > +    template<typename T> std::optional<T> readAs(const char* name)
> > > +    {
> > > +        if (const char* valueStr = ::getenv(name)) {
> > > +            T value;
> > > +            if (sscanf(valueStr, sscanTemplate<T>(), &value) == 1)
> > > +                return value;
> > > +        }
> > > +
> > > +        return std::nullopt;
> > > +    }
> > > +
> > > +private:
> > > +    template<typename T> const char* sscanTemplate()
> > > +    {
> > > +        ASSERT_NOT_REACHED();
> > > +        return nullptr;
> > > +    }
> > > +
> > > +    template<> const char* sscanTemplate<unsigned>() { return "%u"; }
> > > +};
> > > +
> > 
> > This class could also be put in a header file for reuse. Perhaps we also
> > could consider using a namespace instead?
> 
> I put this code here in implementation because I shame on using environment
> variables. Are there any case we gonna use this pattern? We will remove this
> once we switched to WK2.

I think there are several places where 'getenv' is used, but this could potentially be a follow-up patch.

> 
> 
> > 
> > > Source/WebCore/platform/network/curl/CurlContext.cpp:483
> > > +    unsigned value = time >= 0.0 ? static_cast<unsigned>(time) : 0;
> > 
> > For the sake of clarity, I think we should use parentheses here: (time >=
> > 0.0)
> 
> Or move entire conditional statement inside statice_cast:
> 
> unsigned value = static_cast<unsigned>(time >= 0.0 ? time : 0);
> 
> Much easier to read. What do you think?

Yes, I think this looks good. You could also use 'auto'.

[Basuke Suzuki]

Created attachment 334833 [details]
PATCH

[WebKit Commit Bot]

Comment on attachment 334833 [details]
PATCH

Clearing flags on attachment: 334833

Committed r229138: <https://trac.webkit.org/changeset/229138>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38036900>