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 Different request paths share the same fs cache is some cases. #1843

Merged
merged 1 commit into from
Aug 31, 2024

Conversation

newacorn
Copy link
Contributor

@newacorn newacorn commented Aug 25, 2024

For requests with paths that do not have a trailing slash, we always redirect to the corresponding path with a trailing slash. This approach avoids assuming that users will definitely be redirected to the trailing-slash path and instead ensures that we return a proper redirection, checking for an index file or generating a directory listing. We do this because we should not make assumptions about user behavior. Similarly, the cache for dir/ requests should not be used for dir requests.
This issue is also reflected in this issue: #1839

~/GolandProjects/cbrotli_bug/fasthttp fasthttp_test_issue2 56s
❯ go test -race --count=2 .
--- FAIL: TestZstdBytesConcurrent (1.03s)
    zstd_test.go:22: timeout
--- FAIL: TestZstdCompressConcurrent (1.02s)
    zstd_test.go:68: timeout
--- FAIL: TestServeFileDirectoryRedirect (0.00s)
    fs_test.go:926: Unexpected status code 200 for directory '/fasthttputil' without trailing slash. Expecting 302.
--- FAIL: TestZstdBytesConcurrent (1.00s)
    zstd_test.go:22: timeout
FAIL
FAIL	github.com/valyala/fasthttp	16.511s
FAIL

Currently, the implementation may result in shared fs cache between requests for dir and dir/.
@newacorn newacorn changed the title Fashthttp fs cache issue Fix Different request paths share the same fs cache is some cases. Aug 26, 2024
@erikdubbelboer erikdubbelboer merged commit b0ea03f into valyala:master Aug 31, 2024
15 checks passed
@erikdubbelboer
Copy link
Collaborator

Nice find, thanks!

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