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

Re enable HttpMetricsHandlerTests in netty5 branch #2337

Merged
merged 18 commits into from Jun 30, 2022

Conversation

pderop
Copy link
Member

@pderop pderop commented Jun 28, 2022

Related to #1873

This PR is an attempt to bring back the HttpMetricsHandlerTests into netty5 branch.
mainly, here what has been done:

  • in AbstractHttpClientMetricsHandler.java: it seems that now the processed data must also be extracted from the HttpContent parameter that can be passed as argument. I think that the ByteBufHolder is not used anymore, but I did not remote the test on it.
  • Same thing done for AbstractHttpServerMetricsHandler.java: the HttpContent must be checked from the extractProcessedDataFromBuffer method.
  • in HttpTrafficHandler.java, it seems that the handler can't fire an event after having removed itself from the pipeline, so the removal from the pipeline is now done after the event is fired. See netty DefaultChannelHandlerContext.java
  • In AbstractHttpServerMetricsHandler.write method, sometimes the ChannelOperations seems to not be available anymore during listener invocation. This is probably caused by this netty issue, where listeners are now invoked asynchronously. So the ChannelOperations is retrieved and cached before the listener is added.
  • HttpMetricsHandlerTests.java: the test has been refactored in order to always expect to observe 4 disconnects when H1 is used, and three disconnects when H2 or H2C is used.

@pderop pderop added this to the 2.0.0-M1 milestone Jun 28, 2022
@pderop pderop added the type/enhancement A general enhancement label Jun 28, 2022
@pderop pderop self-assigned this Jun 28, 2022
@pderop
Copy link
Member Author

pderop commented Jun 28, 2022

please do not review for the moment, some tests are failing.

@pderop
Copy link
Member Author

pderop commented Jun 28, 2022

I have temporarily disabled the testRecordingFailsClientSide and testRecordingFailsClientSide tests, and now, we have successful tests for both ubuntu and macos. So, I tend to think that some more works need to be done for the other #2335 PR, which is most likely to be again related (because if I disable the testRecordingFailsClientSide and the testRecordingFailsClientSide, then tests are passing OK).

Now, the remaining failing test is the same one for windows from the #2338.

let's not merge this PR for the moment, I need to get back to the other #2335.

@pderop pderop merged commit 41a33ab into reactor:netty5 Jun 30, 2022
@pderop pderop deleted the netty5-fix-httpmetricshandlertests branch June 30, 2022 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants