feat: send 103 Early Hints responses #95

chalasr · 2022-10-18T23:34:08Z

This makes Vulcain sends 103 Early Hints responses when server-push isn't supported (with preload links).
The patch might be very naive but here we are, first contrib in Go. Please tell me if there is something else to be done.

CLAassistant · 2022-10-18T23:34:13Z

All committers have signed the CLA.

andrerom · 2022-10-21T08:43:16Z

There is a todo comment about early hints on line 251, can it be removed now or more changes needed?

chalasr · 2022-10-21T15:25:43Z

Todo removed, thanks @andrerom.
Also I just added a test, sadly it's failing: The 103 response is sent as expected, but it does not have the Link headers. I must be missing something as that header should be forwarded to the 103 response before it is sent. Any idea?

dunglas · 2022-10-21T15:49:19Z

vulcain.go

@@ -227,6 +227,7 @@ func (v *Vulcain) Apply(req *http.Request, rw http.ResponseWriter, responseBody
 	}
 	if addPreloadToVary {
 		responseHeaders.Add("Vary", "Preload")
+		rw.WriteHeader(http.StatusEarlyHints)


I don't think that it's the good place to send the 103 response (hence the test failure).

This should be done here:

vulcain/vulcain.go

Line 256 in f2d77e9

v.addPreloadHeader(newHeaders, url)

and here:

vulcain/vulcain.go

Line 288 in f2d77e9

v.addPreloadHeader(newHeaders, url)

Maybe should we refactor the code to prevent sending many 103 responses and send it only when all Preload headers have been computed?

Also, I think that Server Push should now be opt-in (through a config option). By default, we should only use 103 responses as they are the new standard. This can be done in another PR of course.

Actually I had to gather preload links added by push() and manually add them to the ResponseWriter header map as they were not for some reason. With this approach, only one 103 response is sent with all Links included.
The code feels a bit hackish but it works as expected so unless you see door for improvement, this is ready to go on my side.

dunglas · 2022-10-22T21:47:42Z

vulcain.go

+	}
+	rw.WriteHeader(http.StatusEarlyHints)
+	// remove extra Link from final response headers
+	rw.Header().Del("Link")


Without this, the final response ends up with an extra - duplicated preload link. If you want to try it out (would be nice): checkout my PR, comment that line and run go test -v -timeout 30s -run PreloadHeader

chalasr · 2022-11-01T16:22:54Z

Also, I think that Server Push should now be opt-in (through a config option). By default, we should only use 103 responses as they are the new standard. This can be done in another PR of course.

Work ongoing in https://github.com/dunglas/vulcain/compare/main...chalasr:vulcain:firstclass-103?expand=1
It'd be great to get this PR resolved/merged first as the WIP depends on it.

pautier · 2022-11-07T14:18:24Z

Also, I think that Server Push should now be opt-in (through a config option). By default, we should only use 103 responses as they are the new standard. This can be done in another PR of course.

Work ongoing in https://github.com/dunglas/vulcain/compare/main...chalasr:vulcain:firstclass-103?expand=1 It'd be great to get this PR resolved/merged first as the WIP depends on it.

Hi guys do you think we can move forward with the @chalasr PR, we need it to use correctly VULCAIN :(

dunglas · 2022-11-07T22:15:59Z

vulcain.go

+		}
+		rw.WriteHeader(http.StatusEarlyHints)
+		// remove extra Link from final response headers
+		rw.Header().Del("Link")


Why is this needed? I'm not sure to well understand what's going on here.

#95 (comment)

Without this, the final response ends up with an extra - duplicated preload link. If you want to try it out (would be nice): checkout my PR, comment that line and run go test -v -timeout 30s -run PreloadHeader

Btw, should the final response actually have links if we already sent them in a 103?

Yes, it even must have them, or downloaded data will be discarded.

chalasr · 2022-12-10T19:58:53Z

Friendly ping :)

dunglas · 2023-01-17T18:56:31Z

I pushed a commit simplifying bit the code. I tested locally with Caddy, and it works as expected, the test failure is because the legacy proxy doesn't seem to support 103 properly and is still used by the test suite.

dunglas · 2023-02-08T22:22:00Z

Now that Go 1.20 is released, it should be possible (I hope), to add proper support for 103 Early Hints to the legacy server that we use in our tests suite using a director function: https://pkg.go.dev/net/http/httputil#ReverseProxy.Director

We'll have to keep supporting Go 1.19 until caddyserver/caddy#5288 is fixed.

Another option would be to add a test for this feature in the Caddy module now that caddyserver/caddy#5187 is merged (we should be able to re-enable the tests for the Caddy module of Mercure by the way, but that's not related to this project 😅).

Even if we follow this path, I would like that we add an example using a director function, at least because it will be displayed in the docs and we want to keep good examples of how to use Vulcain as a library without Caddy.

WDYT?

chalasr · 2023-02-24T11:07:41Z

Works for me, I can be back on this PR in March. Sorry for the late reply

dunglas · 2023-08-03T09:48:22Z

Caddy 2.7 is out. We can now drop support for Go 1.19!

dunglas · 2023-10-16T08:08:52Z

Thanks @chalasr. I fixed a tricky issue and some flaky tests. It's entirely green now 🎉

chalasr · 2023-10-16T14:26:24Z

Thank you!

dunglas approved these changes Oct 19, 2022

View reviewed changes

chalasr changed the title ~~feat: send 103 Early Hints responses along preload links~~ feat: send 103 Early Hints responses Oct 19, 2022

chalasr force-pushed the 103-early-hints branch from c29b18c to 5e037f7 Compare October 21, 2022 15:11

dunglas requested changes Oct 21, 2022

View reviewed changes

chalasr force-pushed the 103-early-hints branch 11 times, most recently from a1c53af to 4d78bc0 Compare October 23, 2022 15:17

dunglas reviewed Oct 23, 2022

View reviewed changes

chalasr force-pushed the 103-early-hints branch 2 times, most recently from ac8b0ca to 90bbeb3 Compare October 23, 2022 21:22

dunglas reviewed Nov 7, 2022

View reviewed changes

chalasr force-pushed the 103-early-hints branch from 90bbeb3 to 408d2dc Compare January 13, 2023 18:11

dunglas force-pushed the 103-early-hints branch from 408d2dc to 8007a60 Compare January 17, 2023 18:53

chalasr force-pushed the 103-early-hints branch 2 times, most recently from 2a810b0 to 07bb98d Compare February 5, 2023 15:51

chalasr force-pushed the 103-early-hints branch from 07bb98d to 32c5aa1 Compare February 5, 2023 16:41

dunglas force-pushed the 103-early-hints branch from 32c5aa1 to 4c4d5d4 Compare October 15, 2023 15:16

chalasr and others added 5 commits October 16, 2023 09:41

feat: send 103 Early Hints responses

10128d1

test: simplify tests

c4a5b99

fix: simplify the code and allow easy testing with Caddy

950344d

fix: skip early hints test for now - inconsistent behavior in Go 1.19

28b84b9

various fixes

ccc3996

dunglas force-pushed the 103-early-hints branch from 4c4d5d4 to ccc3996 Compare October 16, 2023 07:42

fix flaky test

d6b698b

dunglas approved these changes Oct 16, 2023

View reviewed changes

dunglas merged commit 6329c52 into dunglas:main Oct 16, 2023
6 checks passed

dunglas mentioned this pull request Oct 16, 2023

Implement Vulcain directly in API Platform Core api-platform/core#5884

Open

chalasr deleted the 103-early-hints branch October 16, 2023 14:25

chalasr mentioned this pull request Oct 16, 2023

Chrome intends to remove support of Server Push technology #70

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: send 103 Early Hints responses #95

feat: send 103 Early Hints responses #95

chalasr commented Oct 18, 2022 •

edited

Loading

CLAassistant commented Oct 18, 2022 •

edited

Loading

andrerom commented Oct 21, 2022

chalasr commented Oct 21, 2022

dunglas Oct 21, 2022

dunglas Oct 21, 2022

chalasr Oct 22, 2022 •

edited

Loading

dunglas Oct 22, 2022

chalasr Oct 23, 2022 •

edited

Loading

chalasr commented Nov 1, 2022 •

edited

Loading

pautier commented Nov 7, 2022

dunglas Nov 7, 2022

chalasr Nov 8, 2022

dunglas Nov 8, 2022

chalasr commented Dec 10, 2022

dunglas commented Jan 17, 2023

dunglas commented Feb 8, 2023

chalasr commented Feb 24, 2023

dunglas commented Aug 3, 2023

dunglas commented Oct 16, 2023

chalasr commented Oct 16, 2023

feat: send 103 Early Hints responses #95

feat: send 103 Early Hints responses #95

Conversation

chalasr commented Oct 18, 2022 • edited Loading

CLAassistant commented Oct 18, 2022 • edited Loading

andrerom commented Oct 21, 2022

chalasr commented Oct 21, 2022

dunglas Oct 21, 2022

Choose a reason for hiding this comment

dunglas Oct 21, 2022

Choose a reason for hiding this comment

chalasr Oct 22, 2022 • edited Loading

Choose a reason for hiding this comment

dunglas Oct 22, 2022

Choose a reason for hiding this comment

chalasr Oct 23, 2022 • edited Loading

Choose a reason for hiding this comment

chalasr commented Nov 1, 2022 • edited Loading

pautier commented Nov 7, 2022

dunglas Nov 7, 2022

Choose a reason for hiding this comment

chalasr Nov 8, 2022

Choose a reason for hiding this comment

dunglas Nov 8, 2022

Choose a reason for hiding this comment

chalasr commented Dec 10, 2022

dunglas commented Jan 17, 2023

dunglas commented Feb 8, 2023

chalasr commented Feb 24, 2023

dunglas commented Aug 3, 2023

dunglas commented Oct 16, 2023

chalasr commented Oct 16, 2023

chalasr commented Oct 18, 2022 •

edited

Loading

CLAassistant commented Oct 18, 2022 •

edited

Loading

chalasr Oct 22, 2022 •

edited

Loading

chalasr Oct 23, 2022 •

edited

Loading

chalasr commented Nov 1, 2022 •

edited

Loading