Conversation
0143a03 to
01fb3c7
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
01fb3c7 to
545a1ab
Compare
|
The generated output of |
3948217 to
6303fbd
Compare
26ab0db to
fe861bd
Compare
jasnell
left a comment
There was a problem hiding this comment.
Follow-up review findings:
- [HIGH]
NeuterableIoStreammade globally refcounted — affects all existing users - [HIGH]
isDefaultFetchPortlogic semantically wrong for TCP ingress - [HIGH]
exceptionToPropagatehas external linkage; should bestaticor in anonymous namespace - [MEDIUM] Stack-allocated
HttpHeaderTablein connect handler; should reuse existing table - [MEDIUM]
TODO(now)needs resolution before merge - [MEDIUM]
KJ_LOG(WARNING)on every accepted TCP connection is noisy - [MEDIUM] Unused
rewritermember inTcpListener - [LOW] Misnamed variable in
writeOnsetInfotrace code - [LOW] Test decodes chunks twice
- [LOW] ConnectHandler parameter named
connectshadows member name
This review was generated with AI assistance and may contain inaccuracies.
b626b42 to
027d7d8
Compare
7713c75 to
41718b6
Compare
|
I believe I've addressed all suggestions, this is ready for re-review. |
e696e2f to
0b46650
Compare
46a18de to
3b66e00
Compare
946fc30 to
e8b3d5b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6059 +/- ##
==========================================
+ Coverage 70.63% 70.64% +0.01%
==========================================
Files 425 425
Lines 116651 116826 +175
Branches 18853 18881 +28
==========================================
+ Hits 82391 82531 +140
- Misses 23023 23047 +24
- Partials 11237 11248 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
939fd78 to
78df968
Compare
78df968 to
e8d3178
Compare
Requires experimental flag for now. Implemented using a socket-based interface, unlike the original PR. See samples/tcp-ingress for an example
e8d3178 to
04e4c46
Compare
This PR implements support for defining a connect() handler, which is behind an experimental compat flag for now. This is largely based on James' previous PR for this (#1429), but includes changes to switch to the socket-based API, drop the deferred proxying code, add more tests, add proper tracing support etc. I don't have a strong background on how workerd/KJ streams work yet – there might be trivial issues in how streams are being used. You'll notice that this PR differs from the internal spec in that it still provides an event to the tail handler and there are several other issues – see the open questions below where I'm looking for input on how to best address this.
Open questions:
This still provides a ConnectEvent with a cfJson field to the JS connect handler: Based on the internal discussion, a cfJson field should not be needed, but the parameters set to be provided in server.c++ (clientIp, clientPid, clientUid) feel like they might be useful – should there be a different way to provide such metadata? Otherwise I'll change this to just provide a plain socket as the first parameter to the handler (as discussed on the internal spec)[Unchanged from previous PR] The headers provided by the incoming connect() call should be passed through instead of being discarded, right?We currently construct a neuterable stream in ServiceWorkerGlobalScope::connect() without really needing to do so – there should be a cleaner way to get an owned AsyncIoStream?I assume that HTTP response codes are not needed for TCP? This is relevant for the tail worker return event, which has an optional return code.Do we need to use HttpRewriter at all for TCP in server.c++? server.c++'sTcpListenerhas remnants of support for this left, but since it's not dealing with HTTP, rewriting might not be neededWindows tests are failing – perhaps related to EW-9330 Implement connect() handler #6059 (comment)?Miniflare tests are failing – looks like the new code path actually does get enabled for them based on having the "experimental" compat flag. As seen in https://github.com/cloudflare/workerd/actions/runs/22805783114/job/66155261833?pr=6059 (CI run on commit 7713c75 where the wrangler CI jobs were put in a different order), the miniflare tests are the only failing onesAdd support for worker connect handler in miniflare workers-sdk#12775 has been merged, the next daily miniflare should be sufficient to fix CI and unblock this PR.The following assumptions were made in implementing this:
I'm assuming that with the socket-based API, neuterable streams are not needed since deferred proxying does not need to be implemented.We may want TLS support in the future, but this is not included for now.