Modify
Warning Please read the Ticket HowTo before creating or commenting on a ticket. Failure to do so may cause your ticket to be rejected or result in a slower response.

Opened 3 years ago

Closed 3 years ago

#9927 closed Patch - Bug Fix (Fixed)

[HANG] DeviceReadBuffer leaks threads on error untill backend hangs

Reported by: Tony Lill <ajlill@…> Owned by: danielk
Priority: minor Milestone: unknown
Component: MythTV - Recording Version: 0.24-fixes
Severity: medium Keywords:
Cc: Ticket locked: no

Description

If an error causes the DeviceReadBuffer? thread to exit on it's own, the Stop function doesn't reap the thread, and the backend eventually runs out and locks up. The attached patch uses the thread variable itself to determine if a thead_join needs to be done, rather than the running variable, which seems to have slightly different semantics.

Attachments (3)

DRB-leak.patch (3.6 KB) - added by Tony Lill <ajlill@…> 3 years ago.
DRB-leak.patch2 (3.3 KB) - added by Tony Lill <ajlill@…> 3 years ago.
The correct patch
DRB-leak.patch3 (3.6 KB) - added by Tony Lill <ajlill@…> 3 years ago.
Patch using new boolean instead of thread.

Download all attachments as: .zip

Change History (10)

Changed 3 years ago by Tony Lill <ajlill@…>

Changed 3 years ago by Tony Lill <ajlill@…>

The correct patch

comment:1 Changed 3 years ago by danielk

Thanks for catching this!

Can you rewrite this without using the thread variable value to determine if a join is needed? I don't believe that is portable.

comment:2 Changed 3 years ago by Github

Refs #9927. Make sure we stop the DRB thread if we somehow reach the dtor with the thread running & also issue a wake if Start is called when the thread is already running.

Branch: master
Changeset: a5d9c063e82ae4a45364ed57f8b5320b78857030

Changed 3 years ago by Tony Lill <ajlill@…>

Patch using new boolean instead of thread.

comment:3 Changed 3 years ago by Tony Lill <ajlill@…>

Ok, I updated the patch to use a boolean instead of the thread variable.

I also looked at the code in master. It still suffers from the problem that IsRunning? tells you if the thread is active, not if it's been created and requires reaping to free up the resources. I don't know if the other changes to the code make that distinction irrelevant for the issue addressed in this bug, but I think it's still a problem.

I also think that there is potentially another problem in the while loop at the end of the Start function. If the ::run starts and exits before that loop can see the IsRunning? return true, it will be stuck there forever.

comment:4 Changed 3 years ago by wagnerrp

  • Status changed from new to assigned

comment:5 Changed 3 years ago by Github

Refs #9927. In DeviceReadBuffer?, make sure we check dorun when waiting for the run() to start in case it ran to completion (due to error) before we started waiting on it.

This also adds a QWaitCondition to avoid a tight loop waiting for the thread to start up.

Branch: master
Changeset: c74387ced6ae2c0657e25278382283398cc141d9

comment:6 Changed 3 years ago by Github

Refs #9927. Make sure DeviceReadBuffer::videodevice is never QString::null, this will cause the logging to segfault.

Branch: master
Changeset: 9e5a094a0a3c1accfca1131b74f2c9173ba8cdf0

comment:7 Changed 3 years ago by danielk

  • Resolution set to Fixed
  • Status changed from assigned to closed

This was applied 0.24-fixes in [760c8db330134fbd4b084473bace157ea778aa27] but the ticket closing hook didn't fire :(

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'new'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.