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

SplitHTTP: Added headers to signal cache bypass #3652

Merged
merged 7 commits into from
Aug 10, 2024
Merged

Conversation

PoneyClairDeLune
Copy link
Contributor

Getting familiar with the development process of Xray.

  • Added Cache-Control: no-store for servers to disable caching, if the middleboxes respect the web standard. Could potentially stop certain CDNs from teeing response to cache, slowing everything down.
  • Added Cache-Control: no-cache for clients to signal cache hit bypass. Should avoid hitting CDN cache altogether. Can be overridden with self-specified headers.
  • Added Accept: text/event-stream for clients to signal an expectation of streamed responses. Can be overridden with self-specified headers.

@Fangliding
Copy link
Member

Fangliding commented Aug 7, 2024

Splithttp uses different paths for each request, so there should be no need to worry about caching issues
Btw, can these modifications bring improvements after testing

@PoneyClairDeLune
Copy link
Contributor Author

PoneyClairDeLune commented Aug 7, 2024

I already knew SplitHTTP busts cache via paths, but this is not to avoid hitting cache. The headers specified are all to tell middleboxes stop getting involved with cache store altogether. All headers added exist in a standard SSE implementation btw.

@PoneyClairDeLune PoneyClairDeLune changed the title Added headers to signal cache bypass SplitHTTP: Added headers to signal cache bypass Aug 7, 2024
@PoneyClairDeLune
Copy link
Contributor Author

On a side note, I've been told that Fastly still behaves like a snail even after manually enabling streamed responses. I have a hypothesis that it's due to Fastly piping the response into their cache store at the same time, but needs someone to validate.

@mmmray
Copy link
Collaborator

mmmray commented Aug 7, 2024

  • regarding cache-control headers: If this can save some cost, it would be great. Was it tested on a certain CDN? I actually find it strange that an aborted streaming response (or any SSE) would be cached at all. I don't think we should add headers just because the standard says they might do something, it's also a factor in fingerprinting, see SplitHTTP: New padding mechanism #3643
  • regarding Accept: text/event-stream: I don't really understand it. Why is it not enough to initiate streaming from the server and accept everything on the client?
  • regarding "can be overridden with self-specified headers": this is certainly a good idea and should be done for existing default headers, independently of whether new ones are added nevermind, we don't have default request headers at the moment

@RPRX
Copy link
Member

RPRX commented Aug 7, 2024

这个你们讨论吧,主要看有没有解决现实中的问题,我觉得 CDN 等全缓存完再发给客户端的话挺奇葩的,第一次请求得等多久

@Fangliding
Copy link
Member

这个你们讨论吧,主要看有没有解决现实中的问题,我觉得 CDN 等全缓存完再发给客户端的话挺奇葩的,第一次请求得等多久

CDN 等全缓存完再发给客户端的这种情况splithttp会直接不通的(
这个PR的意思是 CDN可能会在流式响应的同时缓存内容造成速度下降(有待验证)

@PoneyClairDeLune
Copy link
Contributor Author

@mmmray

I don't really understand it. Why is it not enough to initiate streaming from the server and accept everything on the client?

It acts like a preflight signal to CDNs to expect a Server-Sent Events stream, existing in both Chrome and Firefox. If (a big if) the CDNs respect this header, the following stream with the MIME text/event-stream shall match the preflight signal greenlighting the streamed response. Added cautiously to match some browser behaviour if CDNs down the line block streams advertising as SSE without an SSE expectation, though until that moment it does seem redundant.

@ll11l1lIllIl1lll
Copy link
Contributor

ll11l1lIllIl1lll commented Aug 8, 2024

If there's a genuine need to avoid CDN caching, I think we could consider using 206 Partial Content with a Content-Type: multipart/byteranges header instead of SSE. RFC 2068 explicitly states that MUST NOT cache 206 (Partial) responses.

@PoneyClairDeLune
Copy link
Contributor Author

Maybe this should just extend to customizable server response scheme down the line? Gives more leeway in case CDNs cut SplitHTTP off in the future.

Though this PR was originally only intended for stopping adding load to the cache stores of CDNs.

@ll11l1lIllIl1lll
Copy link
Contributor

ll11l1lIllIl1lll commented Aug 9, 2024

add meek protocol for extreme cases (don't)

@PoneyClairDeLune
Copy link
Contributor Author

PoneyClairDeLune commented Aug 9, 2024

@ll11l1lIllIl1lll Planning exactly that down the line as an entirely different transport XD.

Customizable Meek with different operation modes, and does not care the states of the underlying connections. Should be a lot faster than Tor Meek though, I intend to uphold the original xSSE Meek PoC.

@RPRX
Copy link
Member

RPRX commented Aug 10, 2024

我觉得尚无测试表明客户端加这两个 header 确实有效的情况下先不加,还得加给 Browser Dialer,可能有 #3643 (comment) 问题

服务端可以加 Cache-Control: no-store,需要把对 .gitignore 的修改删掉

@mmmray
Copy link
Collaborator

mmmray commented Aug 10, 2024

https://github.com/mmmray/Xray-core/tree/splithttp-headers-browser-dialer

here is the branch I had for adding header support to the browser dialer. the server needs to be modified to respond with Access-Control-Allow-Headers: Cache-Control and to not reject OPTIONS requests with 405 method not allowed.

I think it's also ok to add functionality to splithttp that is just missing in the browser dialer. tlsSettings and headers option are already like this.

@RPRX
Copy link
Member

RPRX commented Aug 10, 2024

我觉得主要是服务端加个 Cache-Control: no-store 是有必要的,应该能 prevent overloading the cache,以及 stop CDNs from teeing the response stream into their cache, causing slowdowns,但是客户端加这个若有效,最多只对 GET 有效(middlebox 不会去给 POST 找缓存),而 GET 请求占极少数,以 path 为 key 去查有没有 cache 也并不复杂,不会很耗时,加不加区别不大

而是否给客户端 GET 加 Accept: text/event-stream,要取决于有无测试表明它能穿透更多 middlebox,就像当初给服务端加它

至于这个 PR 现在的代码甚至给客户端 POST 加 Accept: text/event-stream,这个纯属搞笑

@RPRX RPRX merged commit 0c73039 into XTLS:main Aug 10, 2024
36 checks passed
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.

5 participants