Skip to content

winsock, move sendbuf update into cf-socket.c#13763

Closed
icing wants to merge 1 commit intocurl:masterfrom
icing:winsock-move-sendbuf-update
Closed

winsock, move sendbuf update into cf-socket.c#13763
icing wants to merge 1 commit intocurl:masterfrom
icing:winsock-move-sendbuf-update

Conversation

@icing
Copy link
Contributor

@icing icing commented May 24, 2024

  • we updated the WINSOCK sendbuf in the transfer loop, but cf-socket.c seems a much better place.
  • moving code and timestamp into the socket filter

I am not a Windows dev, hope this works as intended.

@icing icing requested review from jay and vszakats May 24, 2024 08:11
@icing icing added Windows Windows-specific tidy-up labels May 24, 2024
@jay
Copy link
Member

jay commented May 24, 2024

I'm seeing some strangeness when i test this, data->conn->writesockfd is -1 for the first call, and more if i step through it in the debugger

curl -v --tls-max 1.2 https://example.com

edit: I've got breakpoints which is slowing things down and causes the 1000 logic to trigger more often but anyway the point is it seems data->conn->writesockfd is not set before at least the first call to cf_socket_send

- we updated the WINSOCK sendbuf in the transfer loop,
  but cf-socket.c seems a much better place.
- moving code and timestamp into the socket filter

I am not a Windows dev, hope this works as intended.
@icing icing force-pushed the winsock-move-sendbuf-update branch from 36a953c to 8c5b576 Compare May 25, 2024 08:26
@icing
Copy link
Contributor Author

icing commented May 25, 2024

I'm seeing some strangeness when i test this, data->conn->writesockfd is -1 for the first call, and more if i step through it in the debugger

curl -v --tls-max 1.2 https://example.com

edit: I've got breakpoints which is slowing things down and causes the 1000 logic to trigger more often but anyway the point is it seems data->conn->writesockfd is not set before at least the first call to cf_socket_send

Thanks for testing. The move was incomplete. The sendbuf update should use the filter's socket ctx->sock and not the writesockfd at the connection. Pushed an update.

Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

This PR is fine as is however I've made a branch modified__winsock-move-sendbuf-update and added a change that renames the functions to be easier to understand, moves the time check into win_update_sndbuf_size and
saves the last sndbuf_size to avoid multiple setsockopt calls every second. I can open a separate PR after you land this one or you're welcome to squash it in.

@jay jay closed this in 0b520e1 May 29, 2024
@jay
Copy link
Member

jay commented May 29, 2024

Thanks. I landed this as is and the follow-up is in #13827.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tidy-up Windows Windows-specific

Development

Successfully merging this pull request may close these issues.

2 participants