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

Fix for crc calculation on upstream subrequest #111

Merged
merged 2 commits into from
May 11, 2024

Conversation

mdineen
Copy link
Contributor

@mdineen mdineen commented Jan 31, 2024

CRC32 values are now correctly stored for upstream requests.

@evanmiller
Copy link
Owner

@mdineen Is this ready to merge? Having trouble following the conversation in #90

@mecampbellsoup
Copy link

Just curious - I haven't worked on many C libraries/projects - but is it common for code changes like those in this PR to not have automated testing?

@evanmiller
Copy link
Owner

There should be automatic CI running – a merge from master will kick it off again.

@mdineen
Copy link
Contributor Author

mdineen commented May 10, 2024

Unfortunately creating an automated test to expose this error requires setting up an upstream. This fix applies a widely used method for preserving the data in memory in a cleanup pool, correcting the bug caused by aggressive cleanup in nginx.

The same method is used in https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_geoip_module.c#L643-L661 for example

@evanmiller I'll write another commit to TitanFile:master to trigger ci.

@evanmiller
Copy link
Owner

CI looks good, merging

@evanmiller evanmiller merged commit 8e65b82 into evanmiller:master May 11, 2024
10 checks passed
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.

3 participants