Skip to content

fix: Handle paginated URLs and add HTTP timeouts#178

Open
jqnatividad wants to merge 1 commit intojaemk:masterfrom
jqnatividad:gh-pagination-fix-redux
Open

fix: Handle paginated URLs and add HTTP timeouts#178
jqnatividad wants to merge 1 commit intojaemk:masterfrom
jqnatividad:gh-pagination-fix-redux

Conversation

@jqnatividad
Copy link
Copy Markdown
Contributor

Release fetching now preserves pagination URLs that already contain query parameters (e.g. from Link headers) instead of unconditionally appending ?per_page=100. My project has more than 100 releases and this fixes it.

In addition, the reqwest HTTP client is configured with a 30s connect timeout and a 300s overall timeout (and imports Duration) to avoid long-hanging requests. These changes fix malformed pagination requests and make network calls more resilient.

Release fetching now preserves pagination URLs that already contain query parameters (e.g. from Link headers) instead of unconditionally appending `?per_page=100`. The reqwest HTTP client is configured with a 30s connect timeout and a 300s overall timeout (and imports Duration) to avoid long-hanging requests. These changes fix malformed pagination requests and make network calls more resilient.
Copy link
Copy Markdown
Collaborator

@GideonBear GideonBear left a comment

Choose a reason for hiding this comment

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

If you split this up I can merge the pagination fix

let client = client_builder.http2_adaptive_window(true).build()?;
let client = client_builder
.http2_adaptive_window(true)
.connect_timeout(Duration::from_secs(30))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should IMO be consistent across reqwest and ureq.
Also don't really want to add this before this is configurable via #133, if someone relies on there not being a timeout they'll be stuck until #133 is implemented.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy. I'll just submit a pagination fix PR then...

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants