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

sendf.c: Makes further attempts at updateconninfo if first failed #1847

Closed
wants to merge 1 commit into from

Conversation

@irl
Copy link

commented Aug 30, 2017

When using non-blocking sockets, such as TCP Fast Open sockets,
connection information may not be available at the time the
socket is "created". For TCP Fast Open, the creation can even be
a NOP as far as the network is concerned.

This change adds tests to the Curl_send and Curl_recv functions
to make further attempts at updating the connection information
for as long as local_port in the connectdata struct is zero.

Once a call to Curl_updateconninfo has succeeded, and so updated
the local_port field, there will be no further calls to
Curl_updateconninfo.

Closes: #1332

@irl irl force-pushed the irl:task/1332 branch from 9b28fc8 to fc51fb2 Aug 30, 2017

@irl

This comment has been minimized.

Copy link
Author

commented Aug 30, 2017

CI seems to have passed (not finished the whole matrix just yet, but I don't see why it won't) and I'm happy that this is the correct fix. Please let me know if any changes are required.

@coveralls

This comment has been minimized.

Copy link

commented Aug 30, 2017

Coverage Status

Coverage decreased (-0.07%) to 73.119% when pulling fc51fb2 on irl:task/1332 into 7ec797b on curl:master.

lib/sendf.c Outdated
if(bytes_written >= 0)
if(bytes_written >= 0) {
/* in the case of non-blocking sockets (e.g. TCP Fast Open) this
information may not have been available at the time of socket

This comment has been minimized.

Copy link
@jay

jay Aug 31, 2017

Member

"this information". I'd just say it in one line if you could like /* if local port was not available on first send (TFO), update conninfo */

otherwise looks fine to me as long as local port once available !=0. is there any sockets library where that doesn't happen? let's see what @bagder and @ghedo think

This comment has been minimized.

Copy link
@irl

irl Aug 31, 2017

Author

the only case where I can think of this happening would be with unix sockets or their NTFS equivalent. I do seem to remember that cURL supports these maybe for proxies (I think I did tor testing with it - maybe it was a patch though).

This comment has been minimized.

Copy link
@bagder

bagder Sep 2, 2017

Member

I think @jay's version of the comment is better as it explains the case better.

We might need some kind of precaution for non-TCP transfers to avoid calling that function many times. curl supports unix socket connections that are done instead of the TCP connection and for those, local_port will remain zero. Also, Curl_updateconninfo doesn't actually do anything for UDP transfers since there's no "connection" then.

This comment has been minimized.

Copy link
@irl

irl Sep 2, 2017

Author

@bagder Would there be any case where primary_port is set but local_port would always be zero? For UDP connections, shouldn't local_port be set during the bind?

Edit: The thinking being to test for a non-zero primary_port also to avoid running on UNIX sockets.

@irl irl force-pushed the irl:task/1332 branch from fc51fb2 to df6da5b Sep 15, 2017

sendf.c: Makes further attempts at updateconninfo if first failed
When using non-blocking sockets, such as TCP Fast Open sockets,
connection information may not be available at the time the
socket is "created". For TCP Fast Open, the creation can even be
a NOP as far as the network is concerned.

This change adds tests to the Curl_send and Curl_recv functions
to make further attempts at updating the connection information
for as long as local_port in the connectdata struct is zero.

Once a call to Curl_updateconninfo has succeeded, and so updated
the local_port field, there will be no further calls to
Curl_updateconninfo.

Closes: #1332

@irl irl force-pushed the irl:task/1332 branch from df6da5b to 1705d63 Sep 15, 2017

@irl

This comment has been minimized.

Copy link
Author

commented Sep 15, 2017

Ok, I think this addresses the review comments and I've rebased on master so should be nice and mergeable. (:

if(bytes_written >= 0) {
/* if local port was not available on first send (TFO), update conninfo */
if(num == 0 && (conn->primary_port != 0) && (conn->local_port == 0))
Curl_updateconninfo(conn, conn->sock[FIRSTSOCKET]);

This comment has been minimized.

Copy link
@bagder

bagder Sep 21, 2017

Member

Curl_updateconninfo() does nothing for SOCK_DGRAM sockets (UDP), so it'll keep calling that function during all UDP transfers. Not a big deal but also not elegant.

Curl_updateconninfo() has a check that only updates those port numbers if not doing TFO, which seems to void this entire approach? Shouldn't that condition also be modified?

This comment has been minimized.

Copy link
@jay

jay Sep 22, 2017

Member

regarding the second point iirc tfo is actually disabled once it is sent, therefore eventually it will be false and conninfo will be updated. anyway i think maybe we should change that so tfo stays true if the connection was established with tfo and have a new bool like tfo_sent or something and then he could do a if(num ==0 && tfo_sent) do update etc

This comment has been minimized.

Copy link
@jay

jay Sep 22, 2017

Member

hm reviewing the related threads just now I see there was a needs_update bit proposed in #1335, why was that abandoned?

This comment has been minimized.

Copy link
@bagder

bagder Sep 23, 2017

Member

Yeah, I would prefer that. It feels a bit counter-intuitive to switch off the boolean that says TFO is requested after it has been "used". I also think we might soon find reasons for us to keep that knowledge around even after the initial data has been sent - for handling request replays properly/differently.

@bagder

This comment has been minimized.

Copy link
Member

commented Jan 28, 2018

This still has outstanding questions/feedback since September last year. If you intend to take up the work again, we can reopen.

@bagder bagder closed this Jan 28, 2018

@jay jay added the mothballed label Jan 28, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.