Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

WritableStream is now flushed prior to startTls. #572

Merged
merged 1 commit into from
Apr 27, 2023
Merged

WritableStream is now flushed prior to startTls. #572

merged 1 commit into from
Apr 27, 2023

Conversation

dom96
Copy link
Collaborator

@dom96 dom96 commented Apr 27, 2023

As discussed in #498 (comment) (and internally, TL;DR: we need to flush as JS can buffer in WritableStream).

This PR implements a new flush method on the WritableStream. This is used prior to calling the tlsStarter which initiates the TLS connection.

Test Plan

With script: https://gist.github.com/dom96/e5424c9c03c2b0c8e2eb97ba63163a73

$ bazel run @workerd//src/workerd/server:workerd -- serve $PWD/deps/workerd/samples/tcp/config.capnp --watch --verbose --experimental
$ curl localhost:8080

Output

Expand 
workerd/io/worker.c++:1543: info: console.log(); message() = ["Closed orig socket: ", null]
workerd/io/worker.c++:1543: info: console.log(); message() = ["Reading.."]
workerd/io/worker.c++:1543: info: console.log(); message() = ["HTTP/1.0 302 Found\r\nLocation: https://www.google.com/sorry/index?continue=https://www.google.com/&q=EgSsRhMXGNmOqqIGIim9qA_vBY4gVUd65gM1QJUdR59zajpQyUGWD97g1efcnopdgyZDw__30jIBcg\r\nDate: Thu, 27 Apr 2023 14:31:53 GMT\r\nPragma: no-cache\r\nExpires: Fri, 01 Jan 1990 00:00:00 GMT\r\nCache-Control: no-store, no-cache, must-revalidate\r\nContent-Type: text/html; charset=UTF-8\r\nServer: HTTP server (unknown)\r\nContent-Length: 349\r\nX-XSS-Protection: 0\r\nAlt-Svc: h3=\":443\"; ma=2592000,h3-29=\":443\"; ma=2592000\r\n\r\n<HTML><HEAD><meta http-equiv=\"content-type\" content=\"text/html;charset=utf-8\">\n<TITLE>302 Moved</TITLE></HEAD><BODY>\n<H1>302 Moved</H1>\nThe document has moved\n<A HREF=\"https://www.google.com/sorry/index?continue=https://www.google.com/&amp;q=EgSsRhMXGNmOqqIGIim9qA_vBY4gVUd65gM1QJUdR59zajpQyUGWD97g1efcnopdgyZDw__30jIBcg\">here</A>.\r\n</BODY></HTML>\r\n"]
workerd/io/worker.c++:1543: info: console.log(); message() = ["Reading.."]
workerd/jsg/util.c++:451: info: exception = kj/compat/tls.c++:382: disconnected: peer disconnected without gracefully ending TLS session
stack: /home/dominik/.cache/bazel/_bazel_dominik/445d134e96183174081e87b0919a0c3f/execroot/edgeworker/bazel-out/k8-dbg/bin/external/workerd/src/workerd/server/workerd@2193050 /home/dominik/.cache/bazel/_bazel_dominik/445d134e96183174081e87b0919a0c3f/execroot/edgeworker/bazel-out/k8-dbg/bin/external/workerd/src/workerd/server/workerd@1ac2090 /home/dominik/.cache/bazel/_bazel_dominik/445d134e96183174081e87b0919a0c3f/execroot/edgeworker/bazel-out/k8-dbg/bin/external/workerd/src/workerd/server/workerd@16f7c80 /home/dominik/.cache/bazel/_bazel_dominik/445d134e96183174081e87b0919a0c3f/execroot/edgeworker/bazel-out/k8-dbg/bin/external/workerd/src/workerd/server/workerd@193f2d0

jasnell
jasnell reviewed Apr 27, 2023
View reviewed changes
src/workerd/api/sockets.c++
KJ_IF_MAYBE(expectedHost, s->expectedServerHostname) {
acceptedHostname = *expectedHost;
auto& context = IoContext::current();
auto secureStreamPromise = context.awaitJs(writable->flush(js).then(js,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens here if the writable errors while trying to flush?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would hope that it gets propagated up as part of the PromisedAsyncIoStream below. Is that a fair assumption?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should. let's be sure to test that case in the starttls tests just to make sure :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, will add to my TODOs.

@dom96 dom96 force-pushed the dominik/flush-writable-before-starttls branch from 74fa56a to a784891 Compare April 27, 2023 15:29
@dom96 dom96 merged commit 22889ad into main Apr 27, 2023
@kentonv kentonv deleted the dominik/flush-writable-before-starttls branch May 1, 2023 21:46
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@kentonv kentonv kentonv left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dom96 @jasnell @kentonv