Skip to content

Conversation

@mechite
Copy link
Contributor

@mechite mechite commented Jan 13, 2026

Enumeration of changes from async-kwik.
This is a draft PR as a cleanup of the very big patch @odanielmeinicke made

Signed-off-by: Mahied Maruf <contact@mechite.com>

odanielmeinicke and others added 4 commits November 26, 2025 20:16
The license header was stripped, a violation of @ptrd's copyright
by distributing this code.

The diff of the related commit was quite large, but it appeared to
not have any other similar violations, so I believe this is the only
one.

Related-to: 95504d9
Signed-off-by: Mahied Maruf <contact@mechite.com>
This reverts commit 95504d9.

Signed-off-by: Mahied Maruf <contact@mechite.com>
@mechite mechite marked this pull request as draft January 13, 2026 03:58
Signed-off-by: Mahied Maruf <contact@mechite.com>
Related-to: 6f2c8ce
Signed-off-by: Mahied Maruf <contact@mechite.com>
Update all usages to use the accessor methods.

Signed-off-by: Mahied Maruf <contact@mechite.com>
- Explicit type over `var`
- Variable name `listener` instead of `l`
- Reduce one level of nesting

Signed-off-by: Mahied Maruf <contact@mechite.com>
This was added to the h09 module and seems to not be necessary.

Signed-off-by: Mahied Maruf <contact@mechite.com>
Signed-off-by: Mahied Maruf <contact@mechite.com>
Signed-off-by: Mahied Maruf <contact@mechite.com>
This moves the pool to the `QuicStreamImpl`, but still continues to
be a package-private implementation detail.

Signed-off-by: Mahied Maruf <contact@mechite.com>
This change copies the OpenJDK 19+ default close method in
`ListenerThreadPool` (for support of older versions).

The `QuicStream#close` method here does not follow the existing TODO
comment, only closes the new `ListenerThreadPool`.

Signed-off-by: Mahied Maruf <contact@mechite.com>
This is equivalent, available since JDK 1.5, and simpler.

Signed-off-by: Mahied Maruf <contact@mechite.com>
This does not appear to have been ever written to, only ever read from.
It was also never guarded against multi-threaded access.

This patch also makes `ListenerThreadPool implements Executor` as a
result of the stream no longer being tracked.

Signed-off-by: Mahied Maruf <contact@mechite.com>
@mechite
Copy link
Contributor Author

mechite commented Jan 13, 2026

I could not get the build system working properly to run all tests, but I resolved all conflicts and minimized the patch.
I am not confident in this PR

@odanielmeinicke 6121bb2 shows a bug in your code?
You never wrote to that collection, I couldn't figure out what you were trying to do with it
Since this pool is making a single-thread executor, I removed the collection entirely

mechite and others added 4 commits January 13, 2026 05:18
Signed-off-by: Mahied Maruf <contact@mechite.com>
Signed-off-by: Mahied Maruf <contact@mechite.com>
grep of `== true` and `== false` in the project returns no
other results, so this is the only one.

Signed-off-by: Mahied Maruf <contact@mechite.com>
This merges two files as-is from the related PR.

It then uses the new `VirtualExecutor` system where the single-thread
executor was previously being used as part of these async changes.

Related-to: ptrd#74
Co-authored-by: Josiah Noel <32279667+SentryMan@users.noreply.github.com>
Signed-off-by: Mahied Maruf <contact@mechite.com>
@odanielmeinicke
Copy link

Hi, i appreciate the time you spent on my async-kwik, you've done a really good job here; But take as a note this code is a pure jerry-rig, i've made it in literally one day just to validate an other project that needed the kwik to be async for scalability, there's no tests and advanced code revisions for it. Anyways, i tested it on my library everyday since i publicated it initially and for now, there's no issues (for now)

As this code is a pure jerry-rig, we can collab each other and create something more elaborated for Kwik. Feel free to contact me in that case.

ME, PERSONALLY, the original creator of these changes DON'T recommend these changes (That's why i didn't pulled originally. Not because of the bugs, that can be tested; but because the mechanic of the listeners as interfaces, there's similar, but better ways to do that) for the official Kwik library.

Again, thank you for your time, you really made a good work. I analyzed every commit.


Also:

6121bb2 shows a bug in your code?

Not exactly, but yes. That's an incomplete feature to prevent stack overflow. As example: If i do a StreamInputStream#read inside a StreamReadListener#read(QuicStream, long), it will call it again in a loop and may break the implementations if not prepared for that (99% of them, and it's hard to create a "protection" system for that), that's also why i didn't recommend it

@mechite
Copy link
Contributor Author

mechite commented Jan 13, 2026

I think with proper testing the changes should be acceptable.

I'll be honest and say that the long/verbose method names like setStreamReadListener rather than e.g. onRead, and perhaps marking of this also as experimental API (if applicable annotations exist), might end up being an advantage to creating new public API that can later also be regressed (and allowing these changes to be pulled easily)

I think that the design of Kwik being the way it is, is probably just because of time constraints.

I just wanted to keep this PR down to:

  • whatever you may have wanted to improve asynchronicity of Kwik
  • provide virtual threads support (combined with Use Virtual Threads when on JDK 24+ #74)
  • use e.g. long where int used, add more atomicity, and test
    (to fix "connections/streams limit" you suggested might be an issue)

...as this can maybe make Kwik scale up more.

Then, maybe in seperate PRs:

  • JSpecify @NullMarked/@Nullable
  • Some kind of JMH benchmark, to help @ptrd for improving performance?

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