-
Notifications
You must be signed in to change notification settings - Fork 115
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
Removed force_close from all downloaders #4453
Conversation
This flag causes large syncs to fail under certain circumstances, because the port range is used up too fast and reusing ports in time_wait is not appreciated by servers. We cannot possibly fix all broken http servers in the wild. fixes pulp#4452
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should try this and see if there are regressions reported by users. It's hard to know what will happen because this fix was introduced to workaround real webservers in the wild, so our automated tests won't help us. Still though, let's try it.
Also FYI, the original issue we worked around was a checksum mismatch bug, so if we start getting those reported we should know this removal could be related.
@mdellweg ty for making this.
@dralley I think you're an expert in our downloaders, wdyt about being second reviewer? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also +1 on this, my only question (and I'm not strongly in favor of requiring this) is whether we ought to stick it behind an off-by-default feature flag just in case.
But either way we could undo the change fairly easily and quickly if we start encountering such issues again. As long as we recognize them as such.
Note that since we generally call remote.get_downloader()
I believe we still end up with a new downloader on every request anyway? And, thus, probably a new session? https://github.com/pulp/pulpcore/blob/main/pulpcore/app/models/repository.py#L507
We'll add the feature flag if it is still encountered as an issue. |
Backporting as requested in AAH-2857 |
Backport to 3.18: 💚 backport PR created✅ Backport PR branch: Backported as #4794 🤖 @patchback |
Backport to 3.21: 💚 backport PR created✅ Backport PR branch: Backported as #4793 🤖 @patchback |
Backport to 3.22: 💚 backport PR created✅ Backport PR branch: Backported as #4796 🤖 @patchback |
Backport to 3.23: 💚 backport PR created✅ Backport PR branch: Backported as #4798 🤖 @patchback |
Backport to 3.39: 💚 backport PR created✅ Backport PR branch: Backported as #4797 🤖 @patchback |
Backport to 3.41: 💚 backport PR created✅ Backport PR branch: Backported as #4795 🤖 @patchback |
This flag causes large syncs to fail under certain circumstances, because the port range is used up too fast and reusing ports in time_wait is not appreciated by servers.
We cannot possibly fix all broken http servers in the wild.
fixes #4452