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

handler: Allow media type parameters in Content-Type headers #1253

Merged
merged 1 commit into from
Mar 10, 2025

Conversation

masewo
Copy link
Contributor

@masewo masewo commented Mar 7, 2025

This pull request includes several changes to the pkg/handler package, focusing on improving the handling of MIME types and adding new tests. The most important changes include fixing a regular expression, updating comments, modifying the MIME type whitelist, and adding new unit tests.

Improvements to MIME type handling:

Testing enhancements:

@masewo masewo force-pushed the main branch 2 times, most recently from 469b083 to 432a76b Compare March 8, 2025 22:12
Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Great to hear that we are now on the same page!

@Acconut
Copy link
Member

Acconut commented Mar 8, 2025

Can you please have a look at the failing tests?

@masewo
Copy link
Contributor Author

masewo commented Mar 8, 2025

non-a-valid-mime-type is now a valid mime type. I changed it to non a valid mime type.

@masewo masewo requested a review from Acconut March 9, 2025 10:21
@masewo
Copy link
Contributor Author

masewo commented Mar 9, 2025

@Acconut Seems like a flaky e2e test. The TestUnexpectedNetworkReset sometimes fails on Windows, because the PATCH and HEAD requests happen too fast after each other, causing the file locker not unlock the lock file fast enough. Happens for me in every 10th run or so.

Adding a bit of delay between PATCH and HEAD seems to let pass the test more reliable, but maybe it also hides some underlying problem.

A conn.(*net.TCPConn).CloseWrite() (flush the sent data) before the RST seems to work also. According to Wireshark the RST gets send anyway. But maybe it is changing the purpose of this test too much.

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@Acconut Acconut changed the title handler: Allow more filetypes as Content-Type handler: Allow media type parameters in Content-Type headers Mar 10, 2025
@Acconut
Copy link
Member

Acconut commented Mar 10, 2025

@Acconut Seems like a flaky e2e test. The TestUnexpectedNetworkReset sometimes fails on Windows, because the PATCH and HEAD requests happen too fast after each other, causing the file locker not unlock the lock file fast enough. Happens for me in every 10th run or so.

Thank you for looking into the failing e2e although it's not caused by your PR here. I have also noticed that it randomly fails on Windows, while working fine on Linux. It seems to be caused by minor differences in Go's or the OS' networking stack on these platforms.

Adding a short delay between the PATCH and HEAD request is fine. The test is about ensuring that tusd saves data even when the TCP connection is reset. This goal will not be compromised by a short delay.

That being said, it seems like tusd on Windows is returning internal server errors where it actually should return ErrFileLocked:

level=ERROR event=InternalServerError method=HEAD path=b31ad07cb800343739b5b9ad38a0e720 requestId="" id=b31ad07cb800343739b5b9ad38a0e720 message="open D:\a\tusd\tusd\internal\e2e\data\b31ad07cb800343739b5b9ad38a0e720.lock: Access is denied."

and

event=InternalServerError method=HEAD path=b2fd0bd224ede04083b048e03372db84 requestId="" id=b2fd0bd224ede04083b048e03372db84 message="open D:\a\tusd\tusd\internal\e2e\data\b2fd0bd224ede04083b048e03372db84.lock: The process cannot access the file because it is being used by another process."

Feel free to open another PR for the delay, if you are interested

@Acconut Acconut merged commit 66b6529 into tus:main Mar 10, 2025
7 checks passed
@Acconut Acconut linked an issue Mar 10, 2025 that may be closed by this pull request
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.

opus files are downloaded as octet-stream
2 participants