Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTPCORE-761: support ExtendedSocketOption in socket #442

Merged
merged 4 commits into from
Nov 6, 2023

Conversation

kkewwei
Copy link
Contributor

@kkewwei kkewwei commented Nov 5, 2023

Support setting TCP_KEEPIDLE, TCP_KEEPINTERVAL, TCP_KEEPCOUNT in Socket.

@ok2c
Copy link
Member

ok2c commented Nov 5, 2023

@kkewwei Also there are test failures with Java 1.8

Error:  Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.008 s <<< FAILURE! - in org.apache.hc.core5.reactor.TestSingleCoreIOReactor
Error:  org.apache.hc.core5.reactor.TestSingleCoreIOReactor.testSetExtendedSocketOption  Time elapsed: 0.007 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <null> but was: <TCP_KEEPIDLE>

@kkewwei kkewwei requested a review from ok2c November 5, 2023 17:48
@kkewwei kkewwei requested a review from ok2c November 6, 2023 01:14
@ok2c ok2c merged commit a708673 into apache:master Nov 6, 2023
9 checks passed
@kkewwei kkewwei deleted the support_extented_socket_option branch November 6, 2023 12:09
@ok2c
Copy link
Member

ok2c commented Nov 22, 2023

@kkewwei I just discovered that the test case you have submitted was failing on Win11 with JRE 11.0.20.

I removed your commit from master. If you have access to Win11 please re-test your PR and re-submit it against the 5.3.x branch

java.lang.UnsupportedOperationException: 'TCP_KEEPIDLE' not supported

	at java.base/sun.nio.ch.SocketChannelImpl.setOption(SocketChannelImpl.java:215)
	at org.apache.hc.core5.reactor.SingleCoreIOReactor.setExtendedSocketOption(SingleCoreIOReactor.java:313)
	at org.apache.hc.core5.reactor.TestSingleCoreIOReactor.testSetExtendedSocketOption(TestSingleCoreIOReactor.java:54)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:727)
	at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
...
$ mvn --version
Apache Maven 3.9.5 (57804ffe001d7215b5e7bcb531cf83df38f93546)
Maven home: C:\Opt\Git\opt\maven
Java version: 11.0.20.1, vendor: Amazon.com Inc., runtime: C:\Opt\Git\opt\java-11
Default locale: en_US, platform encoding: Cp1252
OS name: "windows 11", version: "10.0", arch: "amd64", family: "windows"

@kkewwei
Copy link
Contributor Author

kkewwei commented Dec 12, 2023

@ok2c I don't have access to Win11. let me try to find the reason.

@kkewwei
Copy link
Contributor Author

kkewwei commented Jul 19, 2024

@ok2c, It seems unnecessary to find out why the test fails on windows11.

If the user set parameters and httpcomponents does not support on the platform, then an exception will be thrown , which force the user to remove it.

We just need to add the check like this:

    @Test
    public void testSetExtendedSocketOption() throws IOException {
        final SingleCoreIOReactor reactor = new SingleCoreIOReactor(null, mock(IOEventHandlerFactory.class), IOReactorConfig.DEFAULT, null, null, null);
        final SocketChannel socketChannel = SocketChannel.open();
        final SocketOption<Integer> tcpKeepIdle;
        final SocketOption<Integer> tcpKeepInterval;
        final SocketOption<Integer> tcpKeepCount;
        // check the parameters are supported on the platform.
        if (getExtendedSocketOptionOrNull(TCP_KEEPIDLE) != null) {
            reactor.setExtendedSocketOption(socketChannel, TCP_KEEPIDLE, 100);
            reactor.setExtendedSocketOption(socketChannel, TCP_KEEPINTERVAL, 10);
            reactor.setExtendedSocketOption(socketChannel, TCP_KEEPCOUNT, 10);

            tcpKeepIdle = getExtendedSocketOptionOrNull(TCP_KEEPIDLE);
            tcpKeepInterval = getExtendedSocketOptionOrNull(TCP_KEEPINTERVAL);
            tcpKeepCount = getExtendedSocketOptionOrNull(TCP_KEEPCOUNT);
            Assertions.assertEquals(100, socketChannel.getOption(tcpKeepIdle));
            Assertions.assertEquals(10, socketChannel.getOption(tcpKeepInterval));
            Assertions.assertEquals(10, socketChannel.getOption(tcpKeepCount));
        }
    }

If ok, I will modify the unit test.

@ok2c
Copy link
Member

ok2c commented Jul 19, 2024

If ok, I will modify the unit test.

@kkewwei Yes, that would be enough as far as I am concerned.

@ok2c
Copy link
Member

ok2c commented Jul 19, 2024

@kkewwei You may also consider doing something similar to what TestSSLContextBuilder does to test if the it is being executed on a Win OS variant.

@kkewwei
Copy link
Contributor Author

kkewwei commented Jul 19, 2024

TestSSLContextBuilder

@ok2c I don't quite understand what you mean. could you describe it more clearly? very thank you.

I should test whether it is supported on a Win OS variant? or whether it works when I set successful on a Win OS variant?

@ok2c
Copy link
Member

ok2c commented Jul 19, 2024

@kkewwei I am sorry I could not express myself clearly enough. What I am trying to say that one can use different asserts or even skip some tests / asserts altogether when running on Windows.

https://github.com/apache/httpcomponents-core/blob/master/httpcore5/src/test/java/org/apache/hc/core5/ssl/TestSSLContextBuilder.java#L712

@kkewwei
Copy link
Contributor Author

kkewwei commented Jul 19, 2024

@ok2c, how about like this?

    public void testSetExtendedSocketOption() throws IOException {
        ......
        // 1.Partial versions of jdk1.8 contain TCP_KEEPIDLE, TCP_KEEPINTERVAL, TCP_KEEPCOUNT.
        // 2. Windows may not support TCP_KEEPIDLE, TCP_KEEPINTERVAL, TCP_KEEPCOUNT.
        if (determineJRELevel() > 8 && isWindows() == false) {
            ......
        }
    }

It seems that I can't push code to the pr, If I should create a new pr?

@ok2c
Copy link
Member

ok2c commented Jul 19, 2024

@kkewwei Exactly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants