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

drop cookies.dat, not required, not thread safe #10611

Merged
merged 1 commit into from Oct 2, 2016

Conversation

FernetMenta
Copy link
Contributor

fix http://trac.kodi.tv/ticket/16935

A systemwide storage for cookies makes no sense and is not thread safe. We may have multiple CURL sessions open at once.

@mention-bot
Copy link

@FernetMenta, thanks for your PR! By analyzing the history of the files in this pull request, we identified @notspiff, @mkortstiege and @Rawk to be potential reviewers.

@FernetMenta FernetMenta added Type: Fix non-breaking change which fixes an issue v17 Krypton labels Oct 2, 2016
@kib
Copy link
Member

kib commented Oct 2, 2016

I don't see the benefit in this either. The cookies.dat shared storage doesn't select the correct cookie anyway. Dropping it makes more sense - the cookie should only exist in the session content.

@FernetMenta FernetMenta merged commit 6164bd7 into xbmc:master Oct 2, 2016
@FernetMenta FernetMenta deleted the cookie branch October 2, 2016 19:52
@arnova
Copy link
Member

arnova commented Oct 5, 2016

This may also fix http://trac.kodi.tv/ticket/16849

@arnova
Copy link
Member

arnova commented Oct 5, 2016

Btw. expect breakage for some stuff, this was once implemented for a reason (not by me) I guess.

@FernetMenta
Copy link
Contributor Author

Since having a global cookie store is wrong by design, I don't expect any well designed behaviour to break.

this was once implemented for a reason

maybe, maybe not. the code was very old. maybe the author was not aware that cookie engine could be enabled without having a file, or this is some later feature of libcurl.

@fritsch
Copy link
Member

fritsch commented Oct 5, 2016

+1

@arnova
Copy link
Member

arnova commented Oct 6, 2016

@FernetMenta : Please note that I'm fine with this change nevertheless since this obviously can never work for multithread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix non-breaking change which fixes an issue v17 Krypton
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants