You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "masaori335 (via GitHub)" <gi...@apache.org> on 2025/09/11 00:31:34 UTC
[PR] Fix background fill on TLS connections [trafficserver]
masaori335 opened a new pull request, #12502:
URL: https://github.com/apache/trafficserver/pull/12502
Fix #12473
Prior to this change, EOS event handling in `HttpSM::state_watch_for_client_abort` was completely different from underlaying protocols. If it's TLS and client aborts early, ATS kills HttpTunnel and terminates HttpSM immediately. Which means ATS doesn't wait response from origin server and the background fill feature can't kick in. This change the behavior to almost the same as plaintext HTTP.
TLS 1.3 has half-open support, so we should implement it in `SSLNetVConnection`, but it's out of scope from this PR.
Co-authored-by Masakazu Kitajo <ma...@apache.org>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix background fill on TLS connections [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 merged PR #12502:
URL: https://github.com/apache/trafficserver/pull/12502
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix background fill on TLS connections [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #12502:
URL: https://github.com/apache/trafficserver/pull/12502#discussion_r2353094971
##########
tests/gold_tests/h2/http2_rst_stream.test.py:
##########
@@ -58,7 +58,8 @@
tr.Processes.Default.Streams.All += Testers.ContainsExpression(
'Submitted RST_STREAM frame for key 1 on stream 1.', 'Send RST_STREAM frame.')
-server.Streams.All += Testers.ExcludesExpression('RST_STREAM', 'Server is not affected.')
+# NOTE: Background Fill fix changed this behavior
+# server.Streams.All += Testers.ExcludesExpression('RST_STREAM', 'Server is not affected.')
Review Comment:
Should this be a ContainsExpression then? Otherwise, maybe best just to remove these lines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix background fill on TLS connections [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on code in PR #12502:
URL: https://github.com/apache/trafficserver/pull/12502#discussion_r2353959449
##########
tests/gold_tests/h2/http2_rst_stream.test.py:
##########
@@ -58,7 +58,8 @@
tr.Processes.Default.Streams.All += Testers.ContainsExpression(
'Submitted RST_STREAM frame for key 1 on stream 1.', 'Send RST_STREAM frame.')
-server.Streams.All += Testers.ExcludesExpression('RST_STREAM', 'Server is not affected.')
+# NOTE: Background Fill fix changed this behavior
+# server.Streams.All += Testers.ExcludesExpression('RST_STREAM', 'Server is not affected.')
Review Comment:
Thank you for taking a look, I was wondering what this check should do. Changed to `ContainsExpression` to make sure origin server also received the `RST_STREAM` frame.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix background fill on TLS connections [trafficserver]
Posted by "masaori335 (via GitHub)" <gi...@apache.org>.
masaori335 commented on PR #12502:
URL: https://github.com/apache/trafficserver/pull/12502#issuecomment-3300911432
[approve ci autest 3of4]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix background fill on TLS connections [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #12502:
URL: https://github.com/apache/trafficserver/pull/12502#issuecomment-3303040730
Cherry-picked to 10.1.x branch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix background fill on TLS connections [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #12502:
URL: https://github.com/apache/trafficserver/pull/12502#discussion_r2353087172
##########
src/iocore/net/UnixNetVConnection.cc:
##########
@@ -625,6 +625,11 @@ UnixNetVConnection::net_write_io(NetHandler *nh)
NetState *s = &this->write;
Continuation *c = this->write.vio.cont;
+ if (c == nullptr) {
+ // If Continuatin to callback is nullptr, we can do nothing
Review Comment:
Continuatin -> Continuation
##########
tests/gold_tests/h2/http2_rst_stream.test.py:
##########
@@ -58,7 +58,8 @@
tr.Processes.Default.Streams.All += Testers.ContainsExpression(
'Submitted RST_STREAM frame for key 1 on stream 1.', 'Send RST_STREAM frame.')
-server.Streams.All += Testers.ExcludesExpression('RST_STREAM', 'Server is not affected.')
+# NOTE: Background Fill fix changed this behavior
+# server.Streams.All += Testers.ExcludesExpression('RST_STREAM', 'Server is not affected.')
Review Comment:
Should this be a ContainsExpression then?
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -897,6 +899,49 @@ SSLNetVConnection::do_io_close(int lerrno)
super::do_io_close(lerrno);
}
+void
+SSLNetVConnection::do_io_shutdown(ShutdownHowTo_t howto)
+{
+ if (get_tunnel_type() == SNIRoutingType::BLIND) {
+ // we don't have TLS layer control of blind tunnel
+ UnixNetVConnection::do_io_shutdown(howto);
+ return;
+ }
+
+ switch (howto) {
+ case IO_SHUTDOWN_READ:
+ // No need to call SSL API
Review Comment:
For the comment: probably good to explain why the API should not be called, rather than just say it doesn't need to be called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org