HttpMetricsHandlerTests testServerConnectionsMicrometer still unstable

[pderop]

The HttpMetricsHandlerTests testServerConnectionsMicrometer is still unstable. Sometimes, the following assertion fails because the SERVER_CONNECTIONS_TOTAL is 1, see this failing assertion

Reproduced the problem in github using netty5 branch while validating the multipart porting draft PR.

What I can imagine is the following probable scenario:

1. the testServerConnectionsMicrometer is finishing an HTTP2 test, so the tearDown is in progress, and the client connection pool is disposing, but the server has not yet seen the close, so the SERVER_CONNECTIONS_TOTAL meter is still set to 1
2. now, the testServerConnectionsMicrometer starts a next test using HTTP1.1, but when it waits for the H1 connection to be closed here, then the SERVER_CONNECTIONS_TOTAL is still set to 2, because the connection close done in previous test in not yet seen by the server.
3. So, when the HTTP1.1 socket is closed and see by the server, the latch is counted down, but the SERVER_CONNECTIONS_TOTAL is now set to 1, because the client socket close from previous test is still in progress, and not yet seen by the server.

So, in testServerConnectionsMicrometer, this problem seems to be fully fixed when we dispose the client connection pool in case protocol is HTT2 and when we also wait for the server to see the H2 connection close.

Fixes #2368

[violetagg]

When we dispose a server we also dispose the connection pool. It happens here https://github.com/reactor/reactor-netty/blob/netty5/reactor-netty5-core/src/main/java/reactor/netty5/transport/ServerTransport.java#L528-L543.

Do you think that the new test starts before server dispose? Or is there an issue with server dispose?

[pderop]

When ServerTransport.dispose method call is done, the server connection is indeed closed (the server does not listen anymore on the server port), that's ok.

But when the dispose method returns, I think that the server has not yet seen the client connection close, it will eventually see it, but not immediately. That's what I remember having observed with the debugger last week.

[pderop]

(the failure CI test on windows is unrelated)

[pderop]

@violetagg ,

I have tried to clean up the test, and I have validated it using the multipart PR which consistently reproduces the issue, can you please review it ?

So, this PR does the following:

• some tests were disposing the server uselessly, now, it's not the case anymore
• same for the connection pool provider: some tests were disposing the connection pool multiple times, it's not the case anymore
• the testServerConnectionsMicrometer problem was the following:

1. testServerConnectionsMicrometer is finishing an HTTP2 test, the tearDown method is closing the provider, but the server has not yet seen the connection close. Even if the server is disposed and does not listen anymore on its server port, it still see the old H2 connection
2. the next testServerConnectionsMicrometer is starting with HTTP1, but the SERVER_CONNECTIONS_TOTAL meter is still set to 1 (because the old server has not yet seen the connection close), so the test fails, see here

a first solution was to use a channelGroup, because when a server configured with a channelGroup it does a graceful shutdown at dispose time. But currently, pending client sockets are closed asynchronously, that's why the SERVER_CONNECTIONS_TOTAL meter may remain positive for a short amount of time. So, in addition , the ServerCloseHandler from the test is still used to ensure all sockets are closed on the server (see tearDown method).

[pderop]

Indeed, I have updated the PR like in ITTracingHttpServerDecoratorTest I also have closed the group, like it is done in ITTracingHttpServerDecoratorTest.close (isn't it also necessary ? else I can revert and just shutdown the executor ?)

PS: in HttpServerTests, the executor and the group don't seem to be closed, should something be fixed in HttpServerTests.java (with another PR) ?

[violetagg]

yes go ahead and fix them all

[pderop]

The failed test on windows is not relevant to this PR.

[pderop]

@violetagg ,

thanks for the review !!