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

support reusing of CFile::m_pFile #10658

Merged
merged 1 commit into from Oct 11, 2016

Conversation

Projects
None yet
4 participants
@peak3d
Copy link
Contributor

commented Oct 8, 2016

Dont open CFile on Open again if its already opened

Description

see title

Motivation and Context

Calling multiple times open on a filesystem file without calling Close() after each call lead to memory leaks (m_pFile is not closed / freed).

CCurlfile is not able to handle multiple Open calls -> Close it on start of Open

How Has This Been Tested?

CURLCreate -> CURLOpen -> CURLOpen -> CURLClose

Types of change

  • [x ] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
@FernetMenta

This comment has been minimized.

Copy link
Member

commented Oct 9, 2016

@peak3d this is required for addons, right?
@MartijnKaijser ok for v17?

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Oct 9, 2016

@FernetMenta if you think it's OK no problem.

@FernetMenta FernetMenta added this to the Krypton 17.0-beta4 milestone Oct 9, 2016

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Oct 9, 2016

jenkins build this please

@peak3d

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2016

It's quite safe, and yes I'll use it for inputstream addons if it's merged.
Could also be used for e.g. scrapers as curl can better reuse once opened connections,

@arnova

This comment has been minimized.

Copy link
Member

commented Oct 10, 2016

I'm confused: I thought the idea was that when a file is already open and we perform a re-open we would simply use the old m_pFile. But now it seems we first close the file and then open it again. Isn't the latter less efficient? Or am I missing something?

@peak3d

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2016

Only CCurlFile implements ReOpen(). CCurlFile Close() lets m_state intact but resets all the other things wich needs to be resetted to allow a new Open() call. Open() then opens and writes using m_state with the still valid underlying easy_handle. This leads to reuse of the connection (if url matches the previous call)

IMO its the correct behaviour for a method called ReOpen.

@arnova

This comment has been minimized.

Copy link
Member

commented Oct 11, 2016

Ah yes, now I see what you're trying to do. Looks ok to me.

@peak3d peak3d merged commit 4fe6cc2 into xbmc:master Oct 11, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins4kodi You did a great job. Have a cookie.
Details

@peak3d peak3d deleted the peak3d:ifileopen branch Oct 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.