Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#11341 closed Bug Report - General (fixed)

RemoteFile::Open() can leak MythSockets

Reported by: paulh <mythtv@…> Owned by: cpinkham
Priority: minor Milestone: 0.27
Component: MythTV - General Version: Master Head
Severity: medium Keywords:
Cc: Ticket locked: no


RemoteFile::Open(void) doesn't check to see if it's sockets have already been opened so it's possible for some new sockets to be created without first closing and deleting the old ones.

Things are made worse by the fact that the RemoteFile? constructor will automatically call Open() if a url is passed to it.

These are a few places where a RemoteFile? is created with a url passed to it and later Open() is called :- There is one in MusicSGIODevice which is where I first spotted the problem while playing back tracks from the Music storage group. There is a couple in MetadataDownload? readMXML() and readNFO().

These all cause the Error: Not all threads were shut down properly: Thread MythSocketThread?(-1) is still running warnings when exiting the FE.

Change History (4)

comment:1 Changed 8 years ago by cpinkham

Owner: set to cpinkham
Status: newassigned

comment:2 Changed 8 years ago by Jim Stichnoth <jstichnoth@…>

In ba75244a7f7e412bdb100b7741eaa6a6f0e6649b/mythtv:

Use RemoteFile::isOpen() instead of Open() to check for success.

Calling Open() was unnecessarily creating extra socket connections to
the backend, and the original socket connections from calling Open()
in the constructor were leaking and accumulating in both the frontend
and backend (refs #11341). Refs #11618

comment:3 Changed 8 years ago by Chris Pinkham <cpinkham@…>

Resolution: fixed
Status: assignedclosed

In 4c9dc6d7eebe6853285efbe76a2f81ad80ebbc3c/mythtv:

RemoteFile? leak fix and simplification

RemoteFile::Open() would leak file descriptors if Open() was explicitly
called again after passing a url to the contructor which itself called

This commit does several things:

  • Moves Open() private since RemoteFile? users shouldn't need to Open()
  • Moves Close() private since there's no need to Close() if you can't Open(). Close() is already called in the destructor.
  • Convert the remaining uses of Open() to isOpen() since the connection is already opened in all these cases.
  • Remove a few Close() uses since they were were redundant since we already Close() in the destructor.
  • Removes unused SetURL() functionality. If you can't Open() or Close(), then you need to pass the URL in to the constructor so there's no need.
  • Fixes the actual leak in Open() by checking to see if the sockets are already open. Just in case.... Closes #11341.

NOTE: This does modify the binary ABI version number due to the remotefile.h

changes, so make clean, etc..

comment:4 Changed 8 years ago by cpinkham

Milestone: unknown0.27
Note: See TracTickets for help on using tickets.