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

Workaround for Netty MixedFileUpload bug #7730

Merged
merged 1 commit into from
Jul 22, 2022
Merged

Conversation

yawkat
Copy link
Member

@yawkat yawkat commented Jul 22, 2022

This is a fix for netty/netty#12627 . This patch changes MixedFileUpload to use its own reference count independent of the underlying storage (memory or file), so that the ref count isn't lost when switching storage mode. I copied the DefaultHttpDataFactory and MixedFileUpload from netty to apply this change.

We can revert this change once a netty fix is available (though we should keep the test).

Fixes #7699

This is a fix for netty/netty#12627 . This patch changes MixedFileUpload to use its own reference count independent of the underlying storage (memory or file), so that the ref count isn't lost when switching storage mode. I copied the DefaultHttpDataFactory and MixedFileUpload from netty to apply this change.

We can revert this change once a netty fix is available (though we should keep the test).

Fixes #7699
@yawkat yawkat added this to the 3.6.0 milestone Jul 22, 2022
@sonarcloud
Copy link

sonarcloud bot commented Jul 22, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 8 Code Smells

64.5% 64.5% Coverage
0.0% 0.0% Duplication

@yawkat
Copy link
Member Author

yawkat commented Jul 22, 2022

not fixing the code smells because they're in copied code and i dont want to modify it further than necessary

@yawkat yawkat merged commit 0c08970 into 3.6.x Jul 22, 2022
@yawkat yawkat deleted the mixed-fileupload-workaround branch July 22, 2022 13:47
Copy link
Collaborator

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

Can we target 3.5.x as the base since this is a bug fix and also use the original Netty license headers?

@yawkat
Copy link
Member Author

yawkat commented Jul 25, 2022

i couldnt use the original license headers, because of our checkstyle rules, but it's the same license (apache), just with 'netty project' instead of 'original authors' (which would be netty in this case), so i hope it's fine?

I don't want to target a patch release because imo it's a fairly intrusive change (essentially replacing two classes) for an issue that not many people seem to hit

@sdelamo
Copy link
Collaborator

sdelamo commented Jul 27, 2022

i couldnt use the original license headers, because of our checkstyle rules, but it's the same license (apache), just with 'netty project' instead of 'original authors' (which would be netty in this case), so i hope it's fine?

We have exclusions for licenses in spotless Gradle plugin where forked a file and keep the original license.

In theory,

@sdelamo
Copy link
Collaborator

sdelamo commented Jul 27, 2022

can you create an issue so that we remember to remove these classes once a patch is available in Netty?

@yawkat
Copy link
Member Author

yawkat commented Jul 27, 2022

@sdelamo #7746

Good to know, should I fix the license header, or do you think it's fine since it's such a small difference?

@sdelamo
Copy link
Collaborator

sdelamo commented Jul 27, 2022

Good to know, should I fix the license header, or do you think it's fine since it's such a small difference?

I think the correct thing is to use the original license headers, but I am not a license expert.

Since we are going to delete these files as soon as #7746 is complete, I think it is ok to keep them as they currently are since they are both Apache 2.0.

yawkat added a commit that referenced this pull request Sep 9, 2022
Fixed by #7968 (netty 4.1.81), don't need the workaround anymore.
Fixes #7699
yawkat added a commit that referenced this pull request Sep 9, 2022
Fixed by #7968 (netty 4.1.81), don't need the workaround anymore.
Fixes #7699
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.

None yet

4 participants