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

Curl_quic_idle #8698

Closed
wants to merge 1 commit into from
Closed

Curl_quic_idle #8698

wants to merge 1 commit into from

Conversation

tatsuhiro-t
Copy link
Contributor

@tatsuhiro-t tatsuhiro-t commented Apr 12, 2022

Curl_quic_idle to be called when no HTTP level read or write is
performed. It is a good place to handle timer expiry for QUIC
transport (.e.g, retransmission).

This is kinda straw man, to add a machinery to vquic to handle general timer expiry stuff when curl has no HTTP data to send or receive.

lib/transfer.c Outdated
if(conn->transport == TRNSPRT_QUIC)
result = Curl_quic_idle(data);
if(result)
return result;
Copy link
Member

@danielgustafsson danielgustafsson Apr 13, 2022

Choose a reason for hiding this comment

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

The contract of this API isn't immediately clear to me, do we want library housekeeping functions be able to abort the transfer here? Also, other subsystem housekeeping, like removing expired cookies etc, is pushed to the respective subsystem to perform at leisure, if we introduce something like this maybe we should design something which can offload more generically?

Copy link
Contributor Author

@tatsuhiro-t tatsuhiro-t Apr 13, 2022

Choose a reason for hiding this comment

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

I assume this is QUIC specific which requires its own transport state communication aside from HTTP layer stuff.
I think this is not generic callback and should not be opened to the other subsystems.
In QUIC, failure in Curl_quic_data would be regarded as connection error, so it should be treated like failing to send data.

And yeah, this patch includes a bug that is caused by the infamous missing {}.

Copy link
Member

@danielgustafsson danielgustafsson Apr 14, 2022

Choose a reason for hiding this comment

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

The thing that sort of bothers me is that it feels like a layering violation of Curl_handler, in that before adding this we should perhaps see if this is a problem generically faced (but not currently addressed) which we can solve in a bigger picture context.

This might be completely wrong but I think the discussion is worth having before special casing.

Copy link
Contributor Author

@tatsuhiro-t tatsuhiro-t Apr 14, 2022

Choose a reason for hiding this comment

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

If we do not have this kind of problem before QUIC is introduced, we do not need to make this generic.
QUIC itself is quite special in a sense that it is a proper transport protocol, the same layer of TCP.
Either way, without this kind of facility, curl is unable to handle QUIC properly.

@bagder
Copy link
Member

@bagder bagder commented Apr 14, 2022

Thoughts on this, @ghedo or @nibanks ?

@bagder bagder added the HTTP/3 label Apr 14, 2022
@nibanks
Copy link
Contributor

@nibanks nibanks commented Apr 14, 2022

If you're trying to drive a QUIC stack you need to be able to run at any time IMO, not just if the app has stuff to send. This is why MsQuic drives this stuff itself. It's no trivial matter to drive the code.

@tatsuhiro-t
Copy link
Contributor Author

@tatsuhiro-t tatsuhiro-t commented Apr 14, 2022

If you have something to send, you can do that along with sending application data.
Other than that, it is basically timer-based.

@nibanks
Copy link
Contributor

@nibanks nibanks commented Apr 15, 2022

If you have something to send, you can do that along with sending application data. Other than that, it is basically timer-based.

I agree. But they why not have a mechanism to indicate the next timeout up to curl instead of just polling in certain states? What if tomorrow, something changes and you need curl to poll in some other state, when there isn't application data? For instance, MsQuic supports the ability to configure keep alives sent periodically at the transport layer. In that scenario, there is no app data and no receives, but just a timer. What about idle timeout? So a good general approach is either to always poll or just give the next timeout to curl and it is responsible for calling you back once it elapses.

@tatsuhiro-t
Copy link
Contributor Author

@tatsuhiro-t tatsuhiro-t commented Apr 15, 2022

Curl has Curl_expire function to let QUIC stack set a timer. When it is expired Curl_readwrite is called. Then it checks sockets or something and calling in to more protocol specific handler. But if there is no data to send or write, nothing happens.
The proposed function call is hit in this situation. If I observed correctly, Curl_readwrite is also called periodically with or without timer. This is why ngtcp2 Curl_quic_idle checks timer expiry explicitly. So the current approach is somehow the hybrid of timer based and periodic poll based, but the polling interval is not derived from QUIC stack, it is generally not useful. It might be better to call the function only when timer is expired.

@tatsuhiro-t
Copy link
Contributor Author

@tatsuhiro-t tatsuhiro-t commented Apr 24, 2022

Thank you for the feedback. It seems like we agreed that some kind method is necessary to send QUIC packet when there is no HTTP data to send. The proposed function is called when timer is expired or periodically, and it serves the purpose.

The argument about whether this kind of method should be generic or not, I'd like to make it QUIC specific in order to minimize the impact to the code base. The absence of this kind of functionality suggests that there is no real requirement for non-QUIC use at the moment.

@tatsuhiro-t tatsuhiro-t marked this pull request as ready for review Apr 24, 2022
@tatsuhiro-t tatsuhiro-t changed the title [WIP] Curl_quic_idle Curl_quic_idle Apr 24, 2022
@bagder
Copy link
Member

@bagder bagder commented Apr 24, 2022

@tatsuhiro-t wrote:

The absence of this kind of functionality suggests that there is no real requirement for non-QUIC use at the moment.

I agree with this. If we at a later point in time realize we need this feature somewhere else, we can move it then.

Add Curl_quic_idle which is called when no HTTP level read or write is
performed.  It is a good place to handle timer expiry for QUIC
transport (.e.g, retransmission).
@tatsuhiro-t
Copy link
Contributor Author

@tatsuhiro-t tatsuhiro-t commented May 9, 2022

Fixed commit message, and it is ready to merge.

@bagder
Copy link
Member

@bagder bagder commented May 9, 2022

Cool, thanks. I will just wait and ship 7.83.1 first this Wednesday, just to not rock the boat unnecessarily this close to release.

@tatsuhiro-t
Copy link
Contributor Author

@tatsuhiro-t tatsuhiro-t commented May 9, 2022

Sure, there is no rush. I hope this would resolve some known issues that cause stalled transfer.

@bagder bagder closed this in 6fcd3e6 May 16, 2022
@bagder bagder deleted the quic-idle branch May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HTTP/3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants