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

Refactoring CurlEngine + Add Curl Socket Reuse Support #2604

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

JC-comp
Copy link
Contributor

@JC-comp JC-comp commented Jan 24, 2024

This pull request resolves the CurlEngine refactoring part of #2530. And introduce socket pool support for caching and reusing sockets, addressing the session rate issue discussed in detail at #2585 (reply in thread).

Changes

  • Refactor CurlEngine
    • Add socket cleanup/setup/execution functions
    • Add a Response class for storing the request and response info
  • Caching and reusing sockets
    • Create a socket pool for management and reuse of sockets

Notes

  • Socket caching significantly enhances throughput in my environment but often leads to 429 errors. Should consider introducing a delay for specific actions to alleviate this issue.
  • There are some overlapping changes between the two major modifications, so I submitted them together.

Add socket cleanup/setup/execution
Add response class
@JC-comp JC-comp changed the base branch from master to onedrive-v2.5.0-alpha-5 January 24, 2024 22:50
@abraunegg
Copy link
Owner

@JC-comp

Socket caching significantly enhances throughput in my environment but often leads to 429 errors. Should consider introducing a delay for specific actions to alleviate this issue.

A 429 response is because there are too many requests coming from the same connection to the server.

This is why, the current 'alpha-5' spins up a new API instance + associated curl instance for each upload|download thread (plus any other API query)

The aim here is to find the right balance between threads and 'requests per curl instance total' so that a 429 response is minimised as much as possible.

@abraunegg
Copy link
Owner

@JC-comp

Should consider introducing a delay for specific actions to alleviate this issue.

When a 429 is presented, the response includes a 'retry-after' header, which, with all the handling, should be correctly handled and used before the request is tried again.

@JC-comp
Copy link
Contributor Author

JC-comp commented Jan 25, 2024

@abraunegg

When a 429 is presented, the response includes a 'retry-after' header, which, with all the handling, should be correctly handled and used before the request is tried again.

Not a bug per se, but I experienced throttling for approximately 3~4 minutes while running requests every 1 ~ 2 minutes (over 1K total requests per minute) in the local first mode with a resync. If we won't get banned or incur a penalty added in the 'retry-after' header due to hitting 429 frequently, then it's okay.

@abraunegg
Copy link
Owner

@JC-comp
I have been testing this PR for a while - are there any further planned changes to this PR or is this good to merge into 'alpha-5' ?

@JC-comp
Copy link
Contributor Author

JC-comp commented Jan 26, 2024

@abraunegg
Yes, it's ready to merge into 'alpha-5'. My testing has found no issues currently.

@abraunegg abraunegg merged commit dbe9251 into abraunegg:onedrive-v2.5.0-alpha-5 Jan 26, 2024
@JC-comp JC-comp deleted the refactor branch January 27, 2024 01:05
@abraunegg
Copy link
Owner

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Repository owner locked and limited conversation to collaborators Feb 3, 2024
@abraunegg abraunegg added this to the v2.5.0 milestone Feb 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants