HttpEventCollectorSender.java does not use the timeout values that are configured.

[steyng]

HttpEventCollectorSender.java does not use the timeout values that are configured.

https://github.com/splunk/splunk-library-javalogging/blob/main/src/main/java/com/splunk/logging/HttpEventCollectorSender.java

The Appender may now be configured to make use of timeout values for connection, read and write. Unfortunately, I cannot see where these values are being used in the code to configure the OkHttpClient and changing these values does not appear to make any difference to operation of the client.

I would have expected to see something like the OkHttpClient.Builder connectionTimeout, readTimeout and writeTimeout methods being called to do the config. (https://square.github.io/okhttp/3.x/okhttp/)

Version of this project you're using: 1.9.0
Platform version: Windows 10
Framework version: Adopt OpenJDK 8.0.292.10
Splunk version: 8.0.5
Running test application locally sending data to a Splunk collector.

Similarly to #162, we can end up with a large amount of memory being used if these cannot be tuned.

Example:

1. Configure the SplunkHttp appender url = https://localhost:8088 (does not exist)
2. Have the test application log a number of messages, e.g. 10000.
3. Using something like VisualVM heap dumps you can see that there are a lot of live objects for HttpEventCollectorEventInfo.
4. Without the ability to tune to cope with the Splunk collector not being available, there is a potential to end up with an OOM problem.

[steyng]

The fix should just be a question of adding the following to the private void startHttpClient() method before the last line httpClient = builder.build();, i.e.

        builder.connectTimeout(Duration.ofMillis(TimeoutSettings.DEFAULT_CONNECT_TIMEOUT));
        builder.readTimeout(Duration.ofMillis(TimeoutSettings.DEFAULT_READ_TIMEOUT));
        builder.writeTimeout(Duration.ofMillis(TimeoutSettings.DEFAULT_WRITE_TIMEOUT));
        builder.callTimeout(Duration.ofMillis(TimeoutSettings.DEFAULT_CALL_TIMEOUT));

Note that at present, it appears that without the statements above, the client is just using the defaults for the OkHttpClient , i.e.

callTimeout = 0
connectTimeout = 10 seconds
readTimeout = 10 seconds
writeTimeout = 10 seconds

[macrergate]

I see such statements here add the ability to specify connection timeout parameters, but they have disappeared later.

[steyng]

@macrergate Good pick-up. It would be better to include the parameters as originally defined in that commit.

[steyng]

Pull request #199 has been updated to reflect the original changes as committed by @zenmoto.

[stokpop]

Note that the current default timeouts are as follows in com.splunk.logging.HttpEventCollectorSender.TimeoutSettings:

    public static final long DEFAULT_CONNECT_TIMEOUT = 30000;
    public static final long DEFAULT_WRITE_TIMEOUT = 0; // 0 means no timeout
    public static final long DEFAULT_CALL_TIMEOUT = 0;
    public static final long DEFAULT_READ_TIMEOUT = 0;

Might be a good idea to change these default timeout, because no timeout can cause issues like increased memory usage when no timeouts are used. Suggestion: DEFAULT_CONNECT_TIMEOUT=3_000 and DEFAULT_WRITE_TIMEOUT = DEFAULT_CALL_TIMEOUT = DEFAULT_READ_TIMEOUT = 10_000

[steyng]

I had missed pull request #178. Have aligned this with #178 and added in some of the default timeouts suggested by @stokpop - set timeouts similar to okHttpClient, i.e.

        public static final long DEFAULT_CONNECT_TIMEOUT = 3000;
        public static final long DEFAULT_WRITE_TIMEOUT = 10000; // 0 means no timeout
        public static final long DEFAULT_CALL_TIMEOUT = 0;
        public static final long DEFAULT_READ_TIMEOUT = 10000;

[stokpop]

Thanks for the update @steyng ! Not sure if the DEFAULT_CALL_TIMEOUT = 0 (no timeout) can possibly cause hanging threads/calls, but I suspect not if the other three timeouts (CONNECT, WRITE and READ) have been set.

How can we have this PR merged in a release? What is next step?

[bparmar-splunk]

Hi All,
I am closing this issue as PR for this issue is merged.

Thanks.