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

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

Closed
wants to merge 1 commit into from

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.

None yet

2 participants