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

Fix SSLStageSuite #771

Open
wants to merge 1 commit into
base: series/0.23
Choose a base branch
from
Open

Conversation

danicheg
Copy link
Member

DISCLAIMER: this is a silly monkey-patching-like attempt to resolve #769

That change might make this particular test nonsense, but I will try to share my insights about it.

Prerequisites

Let us introduce the following notation:

name bytes
contentLength s.getBytes.length
packetSize stageEng.getSession.getPacketBufferSize

Test suite says:

/* The detection of splitting the buffer is seen by checking the write
 * output: if its flushing, the output should only be single buffers for
 * a small flush limits. This could break with changes to the SSLStage
 * algorithm
 */

What is the SSLSession#getPacketBufferSize?

According to the docs:

Gets the current size of the largest SSL/TLS/DTLS packet that is expected when using this session.
An SSLEngine using this session may generate SSL/TLS/DTLS packets of any size up to and including the value returned by this method. All SSLEngine network buffers should be sized at least this large to avoid insufficient space problems when performing wrap and unwrap calls.

So what?

When the contentLength gets closer or goes beyond the value of packetSize – we get a failed test. The default value of packetSize is equal to '16709' on the JDK 17. In that particular test, we set SSLStage#maxWrite to 100. So, my hypothesis is if we adjust the value of contentLegth to be greater than SSLStage#maxWrite and less than packetSize we should preserve the sense of the test suite.

Behind the scenes

Nonetheless, I have no idea why this test become broken. 🤷🏻

@mergify mergify bot added series/0.23 PRs targeting 0.23.x module:blaze-core labels Nov 16, 2022
@danicheg
Copy link
Member Author

@rossabaker sincerely sorry for bothering you, but can you dig into that? This issue is a blocker for any new PR at this point :(

@rossabaker
Copy link
Member

I'm a bit concerned that's masking an actual bug. Why shouldn't it work up to 100%?

@danicheg
Copy link
Member Author

I'm a bit concerned that's masking an actual bug. Why shouldn't it work up to 100%?

🤷🏻 What's even weirder, it spontaneously started failing without any actual changes in the codebase.

@rossabaker
Copy link
Member

We started seeing a similar failure on a patch version of Java 11 that went away on another patch. I think it's changes in the JVM's SSL implementation, and not being rigorously pinned on the JDK we build with.

@danicheg
Copy link
Member Author

I think it's changes in the JVM's SSL implementation, and not being rigorously pinned on the JDK we build with.

I had the same thoughts about that, but we run ontemurin@8. I can't remember when I updated the JDK locally, but we can play with that additionally.

@rossabaker
Copy link
Member

An unblocker is we can mark the test flaky.

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

Successfully merging this pull request may close these issues.

2 participants