Skip to content

Conversation

@brbzull0
Copy link
Contributor

@brbzull0 brbzull0 commented Mar 1, 2023

Just passing them into the quche lib.

ts:
  quic:
    no_activity_timeout_in: 1000
    initial_max_data_in: 60000
    initial_max_stream_data_bidi_local_in: 1000
    initial_max_stream_data_bidi_remote_in: 1000
    initial_max_stream_data_uni_in: 4000

values are advertised correctly:

I00000018 0x65b8963682593300 cry remote transport_parameters initial_max_stream_data_bidi_local=1000
I00000018 0x65b8963682593300 cry remote transport_parameters initial_max_stream_data_bidi_remote=1000
I00000018 0x65b8963682593300 cry remote transport_parameters initial_max_stream_data_uni=4000
I00000018 0x65b8963682593300 cry remote transport_parameters max_idle_timeout=1000
I00000018 0x65b8963682593300 cry remote transport_parameters initial_max_data=60000

curl seems to be getting it:

...
* ngtcp2_conn_handle_expiry returned error: ERR_IDLE_CLOSE
* Connection #0 to host 127.0.0.1 left intact
...

implementation:

ts:
  quic:
    no_activity_timeout_in: 1000
    initial_max_data_in: 60000
    initial_max_stream_data_bidi_local_in: 1000
    initial_max_stream_data_bidi_remote_in: 1000
    initial_max_stream_data_uni_in: 4000
@brbzull0 brbzull0 requested a review from maskit March 1, 2023 14:41
@brbzull0 brbzull0 self-assigned this Mar 1, 2023
@brbzull0 brbzull0 added this to the 10.0.0 milestone Mar 1, 2023
@brbzull0
Copy link
Contributor Author

brbzull0 commented Mar 1, 2023

@maskit is ts.quic.no_activity_timeout_in the right field to be used for the max_idle_timeout set by quiche_config_set_max_idle_timeout?

@brbzull0 brbzull0 added the QUIC label Mar 1, 2023
@brbzull0 brbzull0 changed the title QUIC: Make sure the following configuration gets set into the quiche impl. QUIC: Make sure the some of the quic configs get set into the quiche impl. Mar 1, 2023
@brbzull0
Copy link
Contributor Author

brbzull0 commented Mar 1, 2023

[approve ci autest]

@maskit
Copy link
Member

maskit commented Mar 1, 2023

is ts.quic.no_activity_timeout_in the right field to be used for the max_idle_timeout set by quiche_config_set_max_idle_timeout?

Yes, it is. I'm wondering if the setting should take milliseconds. It's millisecond on the spec but we use seconds for most of ATS settings. proxy.config.http.accept_no_activity_timeout for example.

@brbzull0
Copy link
Contributor Author

brbzull0 commented Mar 1, 2023

is ts.quic.no_activity_timeout_in the right field to be used for the max_idle_timeout set by quiche_config_set_max_idle_timeout?

Yes, it is. I'm wondering if the setting should take milliseconds. It's millisecond on the spec but we use seconds for most of ATS settings. proxy.config.http.accept_no_activity_timeout for example.

I think we can do something for this, and instead of be tight to a particular unit we can actually support multipliers like we do for other units like M,K,G

so eventually we can say:

ts:
  quic:
    no_activity_timeout_in: 1s

@brbzull0
Copy link
Contributor Author

brbzull0 commented Mar 1, 2023

[approve ci autest]

@maskit
Copy link
Member

maskit commented Mar 1, 2023

Hmm, I like the idea but I'm not sure if that works for timeouts. It's kinda obvious that we cannot set 500 milli-bytes nor 8.3 bytes as a file/buffer size. Its minimum unit is obviously byte. On the other hand, 125 milliseconds can make sense for some timeout, but users don't know the actual internal precision unless the documentation says it.

I found the setting currently takes millisecond. I guess I used the unit which the spec use without thinking about it much. Let's keep this PR simple and think about the unit specifier at somewhere else.

@brbzull0
Copy link
Contributor Author

brbzull0 commented Mar 1, 2023

Let's keep this PR simple and think about the unit specifier at somewhere else.

sure.
Thanks.

@brbzull0
Copy link
Contributor Author

brbzull0 commented Mar 1, 2023

[approve ci clang-analyzer]

@brbzull0 brbzull0 merged commit 4112787 into apache:master Mar 2, 2023
@SolidWallOfCode
Copy link
Member

Thinking about the unit specifier here - #9485

@SolidWallOfCode SolidWallOfCode mentioned this pull request Mar 2, 2023
cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jun 3, 2024
* asf/master: (28 commits)
  Human readable timestamp for traffic_ctl config status (apache#9440)
  traffic_ctl - Remove legacy/pretty format output options (apache#9471)
  TSAN fix for fake_global_for_ink_queue. (apache#9183)
  libswoc: replace TextView in src/tscore/HostLookup.cc (apache#9437)
  records.yaml: Make sure when a string field is set to NULL then this (apache#9472)
  QUIC: Make sure the some of the quic configs get set into the quiche impl. (apache#9477)
  Replace httpbin with go-httpbin (apache#9475)
  Ethread::process_event(): make sure event mutex is unlocked before freeing event. (apache#9433)
  Use deprecated OpenSSL APIs for MD5 and SHA256 if available (apache#9469)
  Adds a --enable-lto option (Link Time Optimization) (apache#9464)
  traffic_ctl: Add support to monitor metrics. (apache#9423)
  QUIC: Make some adjustment to the qlog configuration (apache#9461)
  Added more debugging output for the xdebug plugin (apache#9467)
  Build: remove configure check for sys/mount.h, use C++17 builtin. (apache#9463)
  Fix SSLSessionDup for old OpenSSL and BoringSSL (apache#9444)
  Run autest tls_hooks17 and tls_hooks18 on BoringSSL build (apache#9455)
  Stabilize autest tls_hook18 (apache#9454)
  Fix parameter parser in ssl_hook_test plugin (apache#9453)
  Use SSL_get1_peer_certificate on OpenSSL3 build (apache#9460)
  records.yaml - Make some changes on the convert script. (apache#9466)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants