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

Threading issue in sf::SoundRecorder #1011

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@Foaly
Contributor

Foaly commented Nov 29, 2015

When you destroy a sf::SoundBufferRecorder while it's still recording you get the following crash

pure virtual method called
terminate called without an active exception

This is because the sf::SoundBufferRecorder::onProcessSamples() method gets deleted while the capturing thread in sf::SoundRecorder i still running and calling it (destructor of derived class
get called first). Simply calling stop() in the destructor fixes it.

This reproduces the crash:

int main() {
    // create the recorder
    sf::SoundBufferRecorder recorder;

    // start the capture
    recorder.start();

    return 0;
}

Once this is merged I will write a little section for the tutorials metioning that you have to call stop() in the destructor of the class deriving from sf::SoundRecorder.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Nov 29, 2015

Member

Once this is merged I will write a little section for the tutorials metioning that you have to call stop() in the destructor of the class deriving from sf::SoundRecorder.

Why would the user need to do this manually? We're in C++, the base class destructor is called automatically.


Not really related to this PR, but I noticed that m_isCapturing is used from two threads without a mutex, which is not ideal...

Member

Bromeon commented Nov 29, 2015

Once this is merged I will write a little section for the tutorials metioning that you have to call stop() in the destructor of the class deriving from sf::SoundRecorder.

Why would the user need to do this manually? We're in C++, the base class destructor is called automatically.


Not really related to this PR, but I noticed that m_isCapturing is used from two threads without a mutex, which is not ideal...

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Nov 29, 2015

Member

I think you only need to call stop() in the destructor of SoundRecorder, since SoundBufferRecorder derives from SoundRecorder so you'd be calling stop() twice.
And neither do you have to call stop() in the example code.

Member

eXpl0it3r commented Nov 29, 2015

I think you only need to call stop() in the destructor of SoundRecorder, since SoundBufferRecorder derives from SoundRecorder so you'd be calling stop() twice.
And neither do you have to call stop() in the example code.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Nov 29, 2015

Contributor

This is not entire correct. The problem here is timing. It is true that the base class constructor is being called when the derived object is destoryed, but between those two calls the capture thread could be actived and try to call a deleted method, which leads to a crash.

Let me explain in detail. Right now it works like this: SoundBufferRecorder::onProcessSamples overwrittes the pure virtual function SoundRecorder::onProcessSamples which is called by the audio capture thread method. When SoundBufferRecorder destructor is called it's methods get deleted. The capturing thread however is still active, since noone called stop(). It the tryes to call SoundBufferRecorder::onProcessSamples, which is deleted and results in the pure virtual function called crashed.
Putting stop() in the base class destructor doesn't solve the problem entirely, because the base class destructor is called after the derived classes destructor. So if the capturing thread is activated after the methods have been deleted by the derived destructor, but before it is stopped by the base class destructor you have the same crash again.
You can try this out by removing stop() here. Now the program will only crash sometimes (unpredictable, scheduler dependant). If you replace that line with sf::sleep(sf::milliseconds(110)); (m_processingInterval + 10 to make sure the thread function is at least called once) the crash happens everytime.
So the stop() call in the derived calls destructor is nesseccary.

tl;dr: The stop() call in the destructor of the derived class is nesseccary, because the capturing thread has to be stopped before the methods of the derived class (which the threads uses) are deleted.

Contributor

Foaly commented Nov 29, 2015

This is not entire correct. The problem here is timing. It is true that the base class constructor is being called when the derived object is destoryed, but between those two calls the capture thread could be actived and try to call a deleted method, which leads to a crash.

Let me explain in detail. Right now it works like this: SoundBufferRecorder::onProcessSamples overwrittes the pure virtual function SoundRecorder::onProcessSamples which is called by the audio capture thread method. When SoundBufferRecorder destructor is called it's methods get deleted. The capturing thread however is still active, since noone called stop(). It the tryes to call SoundBufferRecorder::onProcessSamples, which is deleted and results in the pure virtual function called crashed.
Putting stop() in the base class destructor doesn't solve the problem entirely, because the base class destructor is called after the derived classes destructor. So if the capturing thread is activated after the methods have been deleted by the derived destructor, but before it is stopped by the base class destructor you have the same crash again.
You can try this out by removing stop() here. Now the program will only crash sometimes (unpredictable, scheduler dependant). If you replace that line with sf::sleep(sf::milliseconds(110)); (m_processingInterval + 10 to make sure the thread function is at least called once) the crash happens everytime.
So the stop() call in the derived calls destructor is nesseccary.

tl;dr: The stop() call in the destructor of the derived class is nesseccary, because the capturing thread has to be stopped before the methods of the derived class (which the threads uses) are deleted.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Nov 29, 2015

Member

I see. Question then is, should the base class still call stop(), even though every derived class is required to call it? Seems to me it could potentially hide a missing stop() in the derived class with a race condition.

Member

eXpl0it3r commented Nov 29, 2015

I see. Question then is, should the base class still call stop(), even though every derived class is required to call it? Seems to me it could potentially hide a missing stop() in the derived class with a race condition.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Nov 29, 2015

Contributor

Well that decision is up to you guys I guess. You could leave it in, so even if the user forgets to call stop in the destructor of the derived class, the whole program doesn't crash right away. Chances that the thread is executed between the two destructor calls are pretty slim. But then again you could argue that that hides a programming error and that you rather have a crash to enforce the correct programming on the user.

Contributor

Foaly commented Nov 29, 2015

Well that decision is up to you guys I guess. You could leave it in, so even if the user forgets to call stop in the destructor of the derived class, the whole program doesn't crash right away. Chances that the thread is executed between the two destructor calls are pretty slim. But then again you could argue that that hides a programming error and that you rather have a crash to enforce the correct programming on the user.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Nov 29, 2015

Member

When SoundBufferRecorder destructor is called it's methods get deleted.

The methods themselves are not destroyed, the object and its VTable are; but you're right in that the access to virtual functions and member variables from the thread SoundRecorder::record() are UB as soon as the derived object is destroyed.

Good observation! This explanation should be part of the docs, because it's really not obvious why the base class destructor can't call stop().

You could leave it in, so even if the user forgets to call stop in the destructor of the derived class, the whole program doesn't crash right away.

That's a sneaky way of obscuring potentially application-breaking mistakes. I can only repeat myself:

DO NOT HIDE LOGIC ERRORS!

You don't help anybody by making a dangerous error less noticeable. What we should do is check in the base destructor whether stop() was called, and trigger an assertion if it wasn't.

Member

Bromeon commented Nov 29, 2015

When SoundBufferRecorder destructor is called it's methods get deleted.

The methods themselves are not destroyed, the object and its VTable are; but you're right in that the access to virtual functions and member variables from the thread SoundRecorder::record() are UB as soon as the derived object is destroyed.

Good observation! This explanation should be part of the docs, because it's really not obvious why the base class destructor can't call stop().

You could leave it in, so even if the user forgets to call stop in the destructor of the derived class, the whole program doesn't crash right away.

That's a sneaky way of obscuring potentially application-breaking mistakes. I can only repeat myself:

DO NOT HIDE LOGIC ERRORS!

You don't help anybody by making a dangerous error less noticeable. What we should do is check in the base destructor whether stop() was called, and trigger an assertion if it wasn't.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Nov 29, 2015

Contributor

Sorry I'll try to not hide logic erros in the future. I didn't think of using an assertion. I updated the code.

Good observation! This explanation should be part of the docs[...]

Thanks! I added some documentation to the SoundRecorder.hpp. Should there be more documentation or a better explanation in the code?
Like I said I wanted to write a paragraf in the Threading issue section of the tutorials once this is merged.

Contributor

Foaly commented Nov 29, 2015

Sorry I'll try to not hide logic erros in the future. I didn't think of using an assertion. I updated the code.

Good observation! This explanation should be part of the docs[...]

Thanks! I added some documentation to the SoundRecorder.hpp. Should there be more documentation or a better explanation in the code?
Like I said I wanted to write a paragraf in the Threading issue section of the tutorials once this is merged.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Nov 29, 2015

Member

Should there be more documentation or a better explanation in the code?

Since it's something that users who inherit sf::SoundRecorder might wonder, I would add it to the documentation of the sf::SoundRecorder class.

Like I said I wanted to write a paragraf in the Threading issue section of the tutorials once this is merged.

Good idea, too. It won't hurt to have it in both places :)

Member

Bromeon commented Nov 29, 2015

Should there be more documentation or a better explanation in the code?

Since it's something that users who inherit sf::SoundRecorder might wonder, I would add it to the documentation of the sf::SoundRecorder class.

Like I said I wanted to write a paragraf in the Threading issue section of the tutorials once this is merged.

Good idea, too. It won't hurt to have it in both places :)

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Nov 29, 2015

Contributor

Sorry I don't get it. There is already documentation in sf::SoundRecorder here. Do you want me to add a more detailed description on why the problem is happing there? It might be interessting for users to understand the problem.

Contributor

Foaly commented Nov 29, 2015

Sorry I don't get it. There is already documentation in sf::SoundRecorder here. Do you want me to add a more detailed description on why the problem is happing there? It might be interessting for users to understand the problem.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Nov 29, 2015

Member

For the docs, mentioning it there is enough, but that paragraph is confusing; the method is not deleted. C++ functions, including virtual ones, exist for the entire application duration and are not bound to object lifetimes. You should rather write something like "You must call stop() in the destructor of your derived class, so that the recording thread finishes before your object is destroyed."

Maybe some additional (code comment) explanation in the sf::SoundRecorder base destructor could explain why stop() can't be called at that point, and why an assertion is used instead. Just to avoid that future developers wonder why it was done this way.

Member

Bromeon commented Nov 29, 2015

For the docs, mentioning it there is enough, but that paragraph is confusing; the method is not deleted. C++ functions, including virtual ones, exist for the entire application duration and are not bound to object lifetimes. You should rather write something like "You must call stop() in the destructor of your derived class, so that the recording thread finishes before your object is destroyed."

Maybe some additional (code comment) explanation in the sf::SoundRecorder base destructor could explain why stop() can't be called at that point, and why an assertion is used instead. Just to avoid that future developers wonder why it was done this way.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Nov 30, 2015

Contributor

I updated the documentation. Once it's verifyed I will squash the commits.

Contributor

Foaly commented Nov 30, 2015

I updated the documentation. Once it's verifyed I will squash the commits.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Dec 10, 2015

Contributor

I just realized the VoIP examples probably needs an update. I will try it out on the weekend.

Contributor

Foaly commented Dec 10, 2015

I just realized the VoIP examples probably needs an update. I will try it out on the weekend.

@eXpl0it3r eXpl0it3r added this to the 2.4 milestone Dec 10, 2015

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jan 3, 2016

Contributor

I updated the example. This should be ready for merging now!

Contributor

Foaly commented Jan 3, 2016

I updated the example. This should be ready for merging now!

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jan 3, 2016

Member

Looks good (minor comment above; and maybe we could squash the commits). We have to keep in mind to fix the m_isCapturing race condition in a separate PR.

Member

Bromeon commented Jan 3, 2016

Looks good (minor comment above; and maybe we could squash the commits). We have to keep in mind to fix the m_isCapturing race condition in a separate PR.

@Foaly Foaly changed the title from Threading issue in `sf::SoundRecorder` to Threading issue in sf::SoundRecorder Jan 3, 2016

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jan 3, 2016

Contributor

Thanks for the review. I corrected the grammer mistake and squashed the commits. They were just seperate to make the review easier. Everything should be good to go now 👍
I opened a new issue for the race condition #1038.
edit: rebased onto master

Contributor

Foaly commented Jan 3, 2016

Thanks for the review. I corrected the grammer mistake and squashed the commits. They were just seperate to make the review easier. Everything should be good to go now 👍
I opened a new issue for the race condition #1038.
edit: rebased onto master

// the object is destroyed. It ensures that stop() is called in the
// destructor of the derived class, which makes sure that the recording
// thread finishes before the derived object is destroyed. Otherwise a
// "pure virtual method called" exception is triggered.

This comment has been minimized.

@TankOs

TankOs Jan 26, 2016

Member

DRY; explanation is already fully covered in the assertion error message.

@TankOs

TankOs Jan 26, 2016

Member

DRY; explanation is already fully covered in the assertion error message.

This comment has been minimized.

@Foaly

Foaly Feb 2, 2016

Contributor

Its true that there is some duplication, but I think in this case it should stay in. This is error is definitly not trivial. I think the comment explains a little more detailed whats happening here any also why. It makes it easier to understand for future readers what kind of error is resolved here. If you read the discussion above this comment was specifically asked for.

@Foaly

Foaly Feb 2, 2016

Contributor

Its true that there is some duplication, but I think in this case it should stay in. This is error is definitly not trivial. I think the comment explains a little more detailed whats happening here any also why. It makes it easier to understand for future readers what kind of error is resolved here. If you read the discussion above this comment was specifically asked for.

onStop();
// Notify derived class
onStop();
}

This comment has been minimized.

@TankOs

TankOs Jan 26, 2016

Member

The last two comments seem to be too much for me. It's clearly understandable from code what happens. ;) Also DRY, as the first comment already explains it all.

@TankOs

TankOs Jan 26, 2016

Member

The last two comments seem to be too much for me. It's clearly understandable from code what happens. ;) Also DRY, as the first comment already explains it all.

This comment has been minimized.

@Foaly

Foaly Feb 2, 2016

Contributor

I cleaned it up a bit.

@Foaly

Foaly Feb 2, 2016

Contributor

I cleaned it up a bit.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 26, 2016

Member

Except for some minor documentation things (again, sorry for that :D): 👍 🐈

Member

TankOs commented Jan 26, 2016

Except for some minor documentation things (again, sorry for that :D): 👍 🐈

Make sure the recording thread in sf::SoundRecorder is stopped before…
… sf::SoundBufferRecorder is destroyed.

Fixes a "pure virtual method called" crash.
Also updated the documentation and the VoIP example.
@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Feb 2, 2016

Contributor

Thanks for review the code and the documentation. I implemented your suggestions.

Contributor

Foaly commented Feb 2, 2016

Thanks for review the code and the documentation. I implemented your suggestions.

@eXpl0it3r eXpl0it3r self-assigned this Mar 29, 2016

@eXpl0it3r eXpl0it3r removed the s:undecided label Mar 29, 2016

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 29, 2016

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Mar 29, 2016

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Apr 10, 2016

Member

Merged in 1ee6d1d

Member

eXpl0it3r commented Apr 10, 2016

Merged in 1ee6d1d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment