Discussion:
[Audacity-devel] Bug437 work in progress
Paul Licameli
2016-11-08 18:06:26 UTC
Permalink
I have made a branch in my fork called Bug437. I invite James and others
to examine the code, and Gale and others to build it and test it on various
operating systems.

Handling of disk space exhaustion for recording follows one path, and there
is one other path to handle exhaustion for effects, commands, and various
other operations that are neither, such as, changing track sample format
using the drop-down menu, which can cause creation of many new block files
for a large project.

The first keeps partial results in the Undo history, but the other rolls
the project back to the previous successfully saved undo state.

I added one more handler for running the Benchmark from the command-line --
end users don't care, but completeness requires it.

The amount of code so far is not very large, but I break it up in many
little commits to explain what I am doing to posterity.

I am not yet confident that these fixes are complete, and there is some
"sweeping" going on to make me confident of that, but this sweep is a lot
of study of the code and very little writing.

PRL
James Crook
2016-11-08 21:17:40 UTC
Permalink
I'm a bit concerned that AudioIO::FillBuffers will catch the exception
AND KEEP GOING.
Should it stop the stream??

You do call gAudio->StopStream() from OnTimer(). That call could be
delayed, e.g. the timer event could be delayed if there are lots of
windows events sitting waiting - or we could be recording many tracks
at a high sample rate with small buffers. So AudioIO::FillBuffers()
could try saving another buffer and create another exception, or even
succeed, before the first one has been handled.

It probably won't in practice, but that is relying on timing.

If you do stop further write attempts, you have to be careful about
software play through. Those presumably should continue.




Does this code work yet? As I read it the BadWriteException in
recording is caught in FillBuffers() and not rethrown anywhere, so I am
not yet seeing how a BadWriteException gets thrown to
AudacityApp::OnExceptionInMainLoop()



--James.
Post by Paul Licameli
I have made a branch in my fork called Bug437. I invite James and others
to examine the code, and Gale and others to build it and test it on various
operating systems.
Handling of disk space exhaustion for recording follows one path, and there
is one other path to handle exhaustion for effects, commands, and various
other operations that are neither, such as, changing track sample format
using the drop-down menu, which can cause creation of many new block files
for a large project.
The first keeps partial results in the Undo history, but the other rolls
the project back to the previous successfully saved undo state.
I added one more handler for running the Benchmark from the command-line --
end users don't care, but completeness requires it.
The amount of code so far is not very large, but I break it up in many
little commits to explain what I am doing to posterity.
I am not yet confident that these fixes are complete, and there is some
"sweeping" going on to make me confident of that, but this sweep is a lot
of study of the code and very little writing.
PRL
Paul Licameli
2016-11-08 23:19:40 UTC
Permalink
James, thanks for reviewing.
Post by James Crook
I'm a bit concerned that AudioIO::FillBuffers will catch the exception
AND KEEP GOING.
Should it stop the stream??
Are you familiar with how three threads interact during recording and
playback?

AudioIO::FillBuffers runs in another thread than the main. There is
nowhere else for the exception to go upward. Some message has to be passed
to the main thread by other means.

Perhaps I should worry about the atomicity of changes in the pointer to the
copied exception object.

I believe I can make the "strong" exception safety guarantee for
WaveTrack::Append, though not necessarily for the other more complicated
operations on WaveTrack and WaveClip and Sequence that may suffer from this
exception, like Delete and Paste. I should complete my examination of
those functions to be sure of that -- what happens with the append buffer?
Post by James Crook
You do call gAudio->StopStream() from OnTimer(). That call could be
delayed, e.g. the timer event could be delayed if there are lots of
windows events sitting waiting - or we could be recording many tracks
at a high sample rate with small buffers. So AudioIO::FillBuffers()
could try saving another buffer and create another exception, or even
succeed, before the first one has been handled.
The function is meant to be called twenty times per second, but maybe one
call will have work that takes longer than 1/20 second to finish.

The strong guarantee for failed Append should be enough to convince us that
the repeated exceptions have no net effect on the track, assuming that if
there is one failure to write a buffer file, then all later attempts will
also fail.

That should be so if the cause of failure is lack of disk space. I believe
all block files it attempts to write will have the same length, except for
a short buffer that gets flushed at the end of recording.

That might not be so if the disk suffers flaky intermittent failures for
some strange reason.
Post by James Crook
It probably won't in practice, but that is relying on timing.
I don't know a better place for the main thread to poll for errors found in
the worker thread that executes FillBuffers repeatedly in a loop. Unless I
make a separate thread just for that, but I wonder what other dangers there
would be if that thread is reponsible for stopping recording. (Remember
the separate thead for polling the mouse for scrubbing, but it can't be
enabled on Linux for reasons still unknown?)

As it is done now, the track panel timer routine is where we check for the
cessation of playback and then make appropriate changes in display and
availability of commands. It seemed the right place to check for this
asynchronous stoppage of recording too.

But maybe I should study what sound activated recording does.
Post by James Crook
If you do stop further write attempts, you have to be careful about
software play through. Those presumably should continue.
Presumably the repeated calls to FillBuffers handle that. FillBuffers
moves playback data from disk files and also moves recording data to files.
(There are the RingBuffer structures in between, and a third thread
spawned by the PortAudio library, that executes the callback function
moving between RingBuffers and another memory buffer.)
Post by James Crook
Does this code work yet?
I don't know! I just know that something like it, with all of those
pieces, is necessary.
Post by James Crook
As I read it the BadWriteException in
recording is caught in FillBuffers() and not rethrown anywhere, so I am
not yet seeing how a BadWriteException gets thrown to
AudacityApp::OnExceptionInMainLoop()
And it does not. I put everything related to handling disk exhaustion
during recording purposes in one commit, and I put other handling for disk
exhaustion in synchronous dispatch of commands and events in another
commit. AudacityApp::OnExceptionInMainLoop() is used only in the latter.

PRL
Post by James Crook
--James.
Post by Paul Licameli
I have made a branch in my fork called Bug437. I invite James and others
to examine the code, and Gale and others to build it and test it on
various
Post by Paul Licameli
operating systems.
Handling of disk space exhaustion for recording follows one path, and
there
Post by Paul Licameli
is one other path to handle exhaustion for effects, commands, and various
other operations that are neither, such as, changing track sample format
using the drop-down menu, which can cause creation of many new block
files
Post by Paul Licameli
for a large project.
The first keeps partial results in the Undo history, but the other rolls
the project back to the previous successfully saved undo state.
I added one more handler for running the Benchmark from the command-line
--
Post by Paul Licameli
end users don't care, but completeness requires it.
The amount of code so far is not very large, but I break it up in many
little commits to explain what I am doing to posterity.
I am not yet confident that these fixes are complete, and there is some
"sweeping" going on to make me confident of that, but this sweep is a lot
of study of the code and very little writing.
PRL
------------------------------------------------------------
------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel
James Crook
2016-11-08 23:41:15 UTC
Permalink
Post by Paul Licameli
James, thanks for reviewing.
Post by James Crook
I'm a bit concerned that AudioIO::FillBuffers will catch the exception
AND KEEP GOING.
Should it stop the stream??
Are you familiar with how three threads interact during recording and
playback?
AudioIO::FillBuffers runs in another thread than the main. There is
nowhere else for the exception to go upward. Some message has to be passed
to the main thread by other means.
Perhaps I should worry about the atomicity of changes in the pointer to the
copied exception object.
I believe I can make the "strong" exception safety guarantee for
WaveTrack::Append, though not necessarily for the other more complicated
operations on WaveTrack and WaveClip and Sequence that may suffer from this
exception, like Delete and Paste. I should complete my examination of
those functions to be sure of that -- what happens with the append buffer?
Post by James Crook
You do call gAudio->StopStream() from OnTimer(). That call could be
delayed, e.g. the timer event could be delayed if there are lots of
windows events sitting waiting - or we could be recording many tracks
at a high sample rate with small buffers. So AudioIO::FillBuffers()
could try saving another buffer and create another exception, or even
succeed, before the first one has been handled.
The function is meant to be called twenty times per second, but maybe one
call will have work that takes longer than 1/20 second to finish.
Is it 20 times a second or 5 times a second? I see an mTimer->Start(200)
Post by Paul Licameli
The strong guarantee for failed Append should be enough to convince us that
the repeated exceptions have no net effect on the track, assuming that if
there is one failure to write a buffer file, then all later attempts will
also fail.
I am not yet convinced, but nearly am, with that assumption.
I worry about the possibility of multiple invocations of the error
dialog. Wouldn't be nice to have it pop up, close it, and have it pop
up again immediately.
Post by Paul Licameli
That should be so if the cause of failure is lack of disk space. I believe
all block files it attempts to write will have the same length, except for
a short buffer that gets flushed at the end of recording.
That might not be so if the disk suffers flaky intermittent failures for
some strange reason.
Post by James Crook
It probably won't in practice, but that is relying on timing.
I don't know a better place for the main thread to poll for errors found in
the worker thread that executes FillBuffers repeatedly in a loop. Unless I
make a separate thread just for that, but I wonder what other dangers there
would be if that thread is reponsible for stopping recording. (Remember
the separate thead for polling the mouse for scrubbing, but it can't be
enabled on Linux for reasons still unknown?)
I don't think an extra thread for it is a good idea.
It would for example be possible to not attempt the append IF there was
an uncleared (by the timer) exception.
Post by Paul Licameli
As it is done now, the track panel timer routine is where we check for the
cessation of playback and then make appropriate changes in display and
availability of commands. It seemed the right place to check for this
asynchronous stoppage of recording too.
But maybe I should study what sound activated recording does.
Post by James Crook
If you do stop further write attempts, you have to be careful about
software play through. Those presumably should continue.
Presumably the repeated calls to FillBuffers handle that. FillBuffers
moves playback data from disk files and also moves recording data to files.
(There are the RingBuffer structures in between, and a third thread
spawned by the PortAudio library, that executes the callback function
moving between RingBuffers and another memory buffer.)
I presume that too, but only because filling the playback buffers
happens before reading the record buffers. If it were the other way
round, we would get a glitchy sound on write failure with software
playthrough, even if the playthrough could end gracefully.
Post by Paul Licameli
Post by James Crook
Does this code work yet?
I don't know! I just know that something like it, with all of those
pieces, is necessary.
Post by James Crook
As I read it the BadWriteException in
recording is caught in FillBuffers() and not rethrown anywhere, so I am
not yet seeing how a BadWriteException gets thrown to
AudacityApp::OnExceptionInMainLoop()
And it does not. I put everything related to handling disk exhaustion
during recording purposes in one commit, and I put other handling for disk
exhaustion in synchronous dispatch of commands and events in another
commit. AudacityApp::OnExceptionInMainLoop() is used only in the latter.
PRL
So I am not yet seeing how the user gets to see an error message in the
recording case. So far I can see (by eyeballing the code) that the
recording stops, but I didn't see another place where the error got
reported to the user.

--James.
Paul Licameli
2016-11-09 00:56:22 UTC
Permalink
Post by Paul Licameli
Post by Paul Licameli
James, thanks for reviewing.
Post by James Crook
I'm a bit concerned that AudioIO::FillBuffers will catch the exception
AND KEEP GOING.
Should it stop the stream??
Are you familiar with how three threads interact during recording and
playback?
AudioIO::FillBuffers runs in another thread than the main. There is
nowhere else for the exception to go upward. Some message has to be
passed
Post by Paul Licameli
to the main thread by other means.
Perhaps I should worry about the atomicity of changes in the pointer to
the
Post by Paul Licameli
copied exception object.
I believe I can make the "strong" exception safety guarantee for
WaveTrack::Append, though not necessarily for the other more complicated
operations on WaveTrack and WaveClip and Sequence that may suffer from
this
Post by Paul Licameli
exception, like Delete and Paste. I should complete my examination of
those functions to be sure of that -- what happens with the append
buffer?
Post by Paul Licameli
Post by James Crook
You do call gAudio->StopStream() from OnTimer(). That call could be
delayed, e.g. the timer event could be delayed if there are lots of
windows events sitting waiting - or we could be recording many tracks
at a high sample rate with small buffers. So AudioIO::FillBuffers()
could try saving another buffer and create another exception, or even
succeed, before the first one has been handled.
The function is meant to be called twenty times per second, but maybe one
call will have work that takes longer than 1/20 second to finish.
Is it 20 times a second or 5 times a second? I see an mTimer->Start(200)
The TrackPanel has another timer that is started with the constant
kTimerInterval which is 50.

But you know, scanning the code for EVT_TIMER, I see it used in many
places, but usually with the first macro argument wxID_ANY and there are
many timers that are not constructed with unique IDs. Maybe that's wrong.
Maybe timer routines fire too often because of this nonspecificity.
Post by Paul Licameli
Post by Paul Licameli
The strong guarantee for failed Append should be enough to convince us
that
Post by Paul Licameli
the repeated exceptions have no net effect on the track, assuming that if
there is one failure to write a buffer file, then all later attempts will
also fail.
I am not yet convinced, but nearly am, with that assumption.
I worry about the possibility of multiple invocations of the error
dialog. Wouldn't be nice to have it pop up, close it, and have it pop
up again immediately.
But you see, it is in AudacityProject::OnAudioIOStopRecording only that we
put up the message for failed writes during recording, and that function is
called from AudioIO::StopStream. And that function is invoked from
TrackPanel::OnTimer(), when it polls for an exception having happened and
finds one.

So what might happen is that FillBuffers finds and copies an exception into
gAudioIO more than once in one recording. Only the latest exception is
kept. It's only when the main thread catches up in the timer loop that we
call StopStream once and put up the dialog once. And StopStream finally
clears the stored exception information, after shutting down the PortAudio
stream, and sending the other thread (which calls FillBuffers) a message to
stop what it is doing, and waits for acknowledgment. That part is this:

mAudioThreadShouldCallFillBuffersOnce = true;


while( mAudioThreadShouldCallFillBuffersOnce == true )

{

// LLL: Experienced recursive yield here...once.

wxGetApp().Yield(true); // Pass true for onlyIfNeeded to avoid
recursive call error.

if (mScrubQueue)

mScrubQueue->Nudge();

wxMilliSleep( 50 );

}


All this said, you may be right that I should cause FillBuffers to copy the
exception to gAudioIO not more than once in one recording.
Post by Paul Licameli
Post by Paul Licameli
That should be so if the cause of failure is lack of disk space. I
believe
Post by Paul Licameli
all block files it attempts to write will have the same length, except
for
Post by Paul Licameli
a short buffer that gets flushed at the end of recording.
That might not be so if the disk suffers flaky intermittent failures for
some strange reason.
Post by James Crook
It probably won't in practice, but that is relying on timing.
I don't know a better place for the main thread to poll for errors found
in
Post by Paul Licameli
the worker thread that executes FillBuffers repeatedly in a loop.
Unless I
Post by Paul Licameli
make a separate thread just for that, but I wonder what other dangers
there
Post by Paul Licameli
would be if that thread is reponsible for stopping recording. (Remember
the separate thead for polling the mouse for scrubbing, but it can't be
enabled on Linux for reasons still unknown?)
I don't think an extra thread for it is a good idea.
It would for example be possible to not attempt the append IF there was
an uncleared (by the timer) exception.
Agreed then, we won't go there.
Post by Paul Licameli
Post by Paul Licameli
As it is done now, the track panel timer routine is where we check for
the
Post by Paul Licameli
cessation of playback and then make appropriate changes in display and
availability of commands. It seemed the right place to check for this
asynchronous stoppage of recording too.
But maybe I should study what sound activated recording does.
Post by James Crook
If you do stop further write attempts, you have to be careful about
software play through. Those presumably should continue.
Presumably the repeated calls to FillBuffers handle that. FillBuffers
moves playback data from disk files and also moves recording data to
files.
Post by Paul Licameli
(There are the RingBuffer structures in between, and a third thread
spawned by the PortAudio library, that executes the callback function
moving between RingBuffers and another memory buffer.)
I presume that too, but only because filling the playback buffers
happens before reading the record buffers. If it were the other way
round, we would get a glitchy sound on write failure with software
playthrough, even if the playthrough could end gracefully.
Then I would simply limit the scope of the try-catch. In fact, I will for
clarity.
Post by Paul Licameli
Post by Paul Licameli
Post by James Crook
Does this code work yet?
I don't know! I just know that something like it, with all of those
pieces, is necessary.
Post by James Crook
As I read it the BadWriteException in
recording is caught in FillBuffers() and not rethrown anywhere, so I am
not yet seeing how a BadWriteException gets thrown to
AudacityApp::OnExceptionInMainLoop()
And it does not. I put everything related to handling disk exhaustion
during recording purposes in one commit, and I put other handling for
disk
Post by Paul Licameli
exhaustion in synchronous dispatch of commands and events in another
commit. AudacityApp::OnExceptionInMainLoop() is used only in the
latter.
Post by Paul Licameli
PRL
So I am not yet seeing how the user gets to see an error message in the
recording case. So far I can see (by eyeballing the code) that the
recording stops, but I didn't see another place where the error got
reported to the user.
See other remarks above. Or put a break in
AudacityProject::OnAudioIOStopRecording
and see the stack that leads to it in case of recording without errors.

PRL
Post by Paul Licameli
--James.
------------------------------------------------------------
------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel
James Crook
2016-11-09 11:08:51 UTC
Permalink
Post by Paul Licameli
Post by Paul Licameli
Post by Paul Licameli
James, thanks for reviewing.
Post by James Crook
I'm a bit concerned that AudioIO::FillBuffers will catch the exception
AND KEEP GOING.
Should it stop the stream??
Are you familiar with how three threads interact during recording and
playback?
AudioIO::FillBuffers runs in another thread than the main. There is
nowhere else for the exception to go upward. Some message has to be
passed
Post by Paul Licameli
to the main thread by other means.
Perhaps I should worry about the atomicity of changes in the pointer to
the
Post by Paul Licameli
copied exception object.
I believe I can make the "strong" exception safety guarantee for
WaveTrack::Append, though not necessarily for the other more complicated
operations on WaveTrack and WaveClip and Sequence that may suffer from
this
Post by Paul Licameli
exception, like Delete and Paste. I should complete my examination of
those functions to be sure of that -- what happens with the append
buffer?
Post by Paul Licameli
Post by James Crook
You do call gAudio->StopStream() from OnTimer(). That call could be
delayed, e.g. the timer event could be delayed if there are lots of
windows events sitting waiting - or we could be recording many tracks
at a high sample rate with small buffers. So AudioIO::FillBuffers()
could try saving another buffer and create another exception, or even
succeed, before the first one has been handled.
The function is meant to be called twenty times per second, but maybe one
call will have work that takes longer than 1/20 second to finish.
Is it 20 times a second or 5 times a second? I see an mTimer->Start(200)
The TrackPanel has another timer that is started with the constant
kTimerInterval which is 50.
But you know, scanning the code for EVT_TIMER, I see it used in many
places, but usually with the first macro argument wxID_ANY and there are
many timers that are not constructed with unique IDs. Maybe that's wrong.
Maybe timer routines fire too often because of this nonspecificity.
OK, yes it is 50. I think we need to review timers in 2.1.4.
Post by Paul Licameli
Post by Paul Licameli
Post by Paul Licameli
The strong guarantee for failed Append should be enough to convince us
that
Post by Paul Licameli
the repeated exceptions have no net effect on the track, assuming that if
there is one failure to write a buffer file, then all later attempts will
also fail.
I am not yet convinced, but nearly am, with that assumption.
I worry about the possibility of multiple invocations of the error
dialog. Wouldn't be nice to have it pop up, close it, and have it pop
up again immediately.
But you see, it is in AudacityProject::OnAudioIOStopRecording only that we
put up the message for failed writes during recording, and that function is
called from AudioIO::StopStream. And that function is invoked from
TrackPanel::OnTimer(), when it polls for an exception having happened and
finds one.
So what might happen is that FillBuffers finds and copies an exception into
gAudioIO more than once in one recording. Only the latest exception is
kept. It's only when the main thread catches up in the timer loop that we
call StopStream once and put up the dialog once. And StopStream finally
clears the stored exception information, after shutting down the PortAudio
stream, and sending the other thread (which calls FillBuffers) a message to
mAudioThreadShouldCallFillBuffersOnce = true;
while( mAudioThreadShouldCallFillBuffersOnce == true )
{
// LLL: Experienced recursive yield here...once.
wxGetApp().Yield(true); // Pass true for onlyIfNeeded to avoid
recursive call error.
if (mScrubQueue)
mScrubQueue->Nudge();
wxMilliSleep( 50 );
}
All this said, you may be right that I should cause FillBuffers to copy the
exception to gAudioIO not more than once in one recording.
Ah. I hadn't followed the mListener->. Yes,
AudacityProject::OnAudioIOStopRecording looks a good place for it, since
it's GUI.
Am a little (but only a little) concerned that we effectively have
::wxMessageBox( e.ErrorMessage() );

in three places. We could end up modifying all three places with later
improvements to reporting. It could be argued that if we are using
exceptions at all for this we should do so 'consistently' and handle and
rethrow as necessary. I'm OK with the current approach though for 2.1.3.




AudacityProject::OnAudioIOStopRecording has:

const auto pException = gAudioIO->GetRecordingException();
if (pException)
// At last here, the write exception detected in the worker
thread,
// for conditions such as exhaustion of disk space,
// causes a user-visible alert in the UI thread, after the push of
// the undo stack has saved as much as possible of the recording.
::wxMessageBox( pException->ErrorMessage() );
}

// Write all cached files to disk, if any
mDirManager->WriteCacheToDisk();


So I notice we could theoretically be doing another write immediately
after putting up the error message and so get another error dialog....
I am guessing that is OK in practice too, as if write caching we won't
have done write in the recording so won't have got the BadWriteException....

This is all highlighting your earlier point that the current code is not
using idioms that makes it easy to verify correctness. I am trying to
see if the idioms suggested so far, if used consistently, would protect
against 'reporting disk full twice'. I think not. There are potentially
plenty of legitimate ways that that write exception could be generated
twice. The measures you are taking will probably be fine in practice
for 2.1.3, way better than what we had, and solving the immediate 437
problem.

Longer term we may need in conjunction with RAII idiom, an error
reporting mechanism that has a modeless dialog, that aggregates (counts)
errors, to avoid the potential for the user to have to play whack-a-mole
with error message dialogs. This can probably be regarded as part of a
general principle of converting events indicating changes of state INTO
state as soon as possible (a counting error dialog IS statey, whereas a
show-dialog() call is eventy) - State is easier to reason about than
events.

--James.
Paul Licameli
2016-11-09 12:13:49 UTC
Permalink
Post by Paul Licameli
Post by Paul Licameli
Post by Paul Licameli
Post by Paul Licameli
James, thanks for reviewing.
Post by James Crook
I'm a bit concerned that AudioIO::FillBuffers will catch the exception
AND KEEP GOING.
Should it stop the stream??
Are you familiar with how three threads interact during recording and
playback?
AudioIO::FillBuffers runs in another thread than the main. There is
nowhere else for the exception to go upward. Some message has to be
passed
Post by Paul Licameli
to the main thread by other means.
Perhaps I should worry about the atomicity of changes in the pointer to
the
Post by Paul Licameli
copied exception object.
I believe I can make the "strong" exception safety guarantee for
WaveTrack::Append, though not necessarily for the other more
complicated
Post by Paul Licameli
Post by Paul Licameli
Post by Paul Licameli
operations on WaveTrack and WaveClip and Sequence that may suffer from
this
Post by Paul Licameli
exception, like Delete and Paste. I should complete my examination of
those functions to be sure of that -- what happens with the append
buffer?
Post by Paul Licameli
Post by James Crook
You do call gAudio->StopStream() from OnTimer(). That call could be
delayed, e.g. the timer event could be delayed if there are lots of
windows events sitting waiting - or we could be recording many tracks
at a high sample rate with small buffers. So AudioIO::FillBuffers()
could try saving another buffer and create another exception, or even
succeed, before the first one has been handled.
The function is meant to be called twenty times per second, but maybe
one
Post by Paul Licameli
Post by Paul Licameli
Post by Paul Licameli
call will have work that takes longer than 1/20 second to finish.
Is it 20 times a second or 5 times a second? I see an
mTimer->Start(200)
Post by Paul Licameli
The TrackPanel has another timer that is started with the constant
kTimerInterval which is 50.
But you know, scanning the code for EVT_TIMER, I see it used in many
places, but usually with the first macro argument wxID_ANY and there are
many timers that are not constructed with unique IDs. Maybe that's
wrong.
Post by Paul Licameli
Maybe timer routines fire too often because of this nonspecificity.
OK, yes it is 50. I think we need to review timers in 2.1.4.
Post by Paul Licameli
Post by Paul Licameli
Post by Paul Licameli
The strong guarantee for failed Append should be enough to convince us
that
Post by Paul Licameli
the repeated exceptions have no net effect on the track, assuming that
if
Post by Paul Licameli
Post by Paul Licameli
Post by Paul Licameli
there is one failure to write a buffer file, then all later attempts
will
Post by Paul Licameli
Post by Paul Licameli
Post by Paul Licameli
also fail.
I am not yet convinced, but nearly am, with that assumption.
I worry about the possibility of multiple invocations of the error
dialog. Wouldn't be nice to have it pop up, close it, and have it pop
up again immediately.
But you see, it is in AudacityProject::OnAudioIOStopRecording only that
we
Post by Paul Licameli
put up the message for failed writes during recording, and that function
is
Post by Paul Licameli
called from AudioIO::StopStream. And that function is invoked from
TrackPanel::OnTimer(), when it polls for an exception having happened and
finds one.
So what might happen is that FillBuffers finds and copies an exception
into
Post by Paul Licameli
gAudioIO more than once in one recording. Only the latest exception is
kept. It's only when the main thread catches up in the timer loop that
we
Post by Paul Licameli
call StopStream once and put up the dialog once. And StopStream finally
clears the stored exception information, after shutting down the
PortAudio
Post by Paul Licameli
stream, and sending the other thread (which calls FillBuffers) a message
to
Post by Paul Licameli
mAudioThreadShouldCallFillBuffersOnce = true;
while( mAudioThreadShouldCallFillBuffersOnce == true )
{
// LLL: Experienced recursive yield here...once.
wxGetApp().Yield(true); // Pass true for onlyIfNeeded to avoid
recursive call error.
if (mScrubQueue)
mScrubQueue->Nudge();
wxMilliSleep( 50 );
}
All this said, you may be right that I should cause FillBuffers to copy
the
Post by Paul Licameli
exception to gAudioIO not more than once in one recording.
Ah. I hadn't followed the mListener->. Yes,
AudacityProject::OnAudioIOStopRecording looks a good place for it, since
it's GUI.
Am a little (but only a little) concerned that we effectively have
::wxMessageBox( e.ErrorMessage() );
in three places. We could end up modifying all three places with later
improvements to reporting. It could be argued that if we are using
exceptions at all for this we should do so 'consistently' and handle and
rethrow as necessary. I'm OK with the current approach though for 2.1.3.
What do you mean? The three places mentioned stop the propagation of error
with a recovery. (Actually one of them is not a catch, because of the
problem of communicating the exception inter-thread.)

Do you worry about all the intermediate levels of stack that get unwound
needing to catch and rethrow? Well no, not if they use RAII properly, but
then there are many such places and that is what I am doubtful of.

Are these three catches complete? I do not yet know. I have not finished
the examination of the code. I have to discover a very large ascending
call graph from NewSimpleBlockFile. I have been writing that in a text
file and it gets pretty big.
Post by Paul Licameli
const auto pException = gAudioIO->GetRecordingException();
if (pException)
// At last here, the write exception detected in the worker
thread,
// for conditions such as exhaustion of disk space,
// causes a user-visible alert in the UI thread, after the push of
// the undo stack has saved as much as possible of the recording.
::wxMessageBox( pException->ErrorMessage() );
}
// Write all cached files to disk, if any
mDirManager->WriteCacheToDisk();
So I notice we could theoretically be doing another write immediately
after putting up the error message and so get another error dialog....
I am guessing that is OK in practice too, as if write caching we won't
have done write in the recording so won't have got the
BadWriteException....
That is not so. I have explained why not. We are sure that recording and
playback have been stopped by AudioIO::StopStream before we reach that
point. No other recording can have started yet because all calls to start
and stop it are serialized in the main thread.
Post by Paul Licameli
This is all highlighting your earlier point that the current code is not
using idioms that makes it easy to verify correctness. I am trying to
see if the idioms suggested so far, if used consistently, would protect
against 'reporting disk full twice'. I think not. There are potentially
plenty of legitimate ways that that write exception could be generated
twice. The measures you are taking will probably be fine in practice
for 2.1.3, way better than what we had, and solving the immediate 437
problem.
I still do not understand this fear. The dialog will happen not more than
once per recording. When it happens, recording will have been stopped.
Post by Paul Licameli
Longer term we may need in conjunction with RAII idiom, an error
reporting mechanism that has a modeless dialog, that aggregates (counts)
errors, to avoid the potential for the user to have to play whack-a-mole
with error message dialogs. This can probably be regarded as part of a
general principle of converting events indicating changes of state INTO
state as soon as possible (a counting error dialog IS statey, whereas a
show-dialog() call is eventy) - State is easier to reason about than
events.
--James.
Did I not make it clear that recording stops after any exception is
detected by the main thread, and a dialog is put up only by the main thread
(it can't be otherwise)? And when it is detected, recording is first
stopped, and partial recording results are saved, before you see the dialog
at all.

The RAII business is another thing -- making sure the rest of the project
state stays consistent and resources do not leak.

PRL
Post by Paul Licameli
------------------------------------------------------------
------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel
James Crook
2016-11-09 16:01:06 UTC
Permalink
Post by Paul Licameli
Post by James Crook
Am a little (but only a little) concerned that we effectively have
::wxMessageBox( e.ErrorMessage() );
in three places. We could end up modifying all three places with later
improvements to reporting. It could be argued that if we are using
exceptions at all for this we should do so 'consistently' and handle and
rethrow as necessary. I'm OK with the current approach though for 2.1.3.
What do you mean? The three places mentioned stop the propagation of error
with a recovery. (Actually one of them is not a catch, because of the
problem of communicating the exception inter-thread.)
The concern is it's not DRY.
https://en.wikipedia.org/wiki/Don%27t_repeat_yourself
It's true ::wxMessageBox is only a one liner, and if we later make it a
custom error dialog we will just call the function for that instead.
Nevertheless a part of me is concerned that 'that code to report an
exception to the user' is in three places.
Post by Paul Licameli
Do you worry about all the intermediate levels of stack that get unwound
needing to catch and rethrow? Well no, not if they use RAII properly, but
then there are many such places and that is what I am doubtful of.
It's more an instinct that the ::wxMessageBox should (maybe) be a catch
right at the top.

I think the 'correct' design is that the original exception actually
stops recording NOT the TrackPanel Timer event. The TrackPanel Timer
Event should only be concerned with GUI things, such as making sure the
record button pops up ... and signalling upwards that an error
reporting dialog for the user is needed.

You're signalling the BadWriteException for recording 'upwards' by
testing state, but elsewhere in the GUI code are using an exception to
signal an error in operations upwards. That's why I am talking about
rethrowing - as a way to merge the two approaches. I get that in the
other cases you undo the partial operation, and that that is not right
for the record case. Maybe a flag is needed in the exception that can
be cleared before rethrowing to control that?

It's not that I worry that the code you've written won't work. Rather it
is about separation of concerns - Recording thread stops the recording;
Repainting Timer ensures visuals are right e.g. button pop ups; Top
level catch ensures error is reported to user. Of course we do currently
put more in our GUI thread than we should so it's not as if it's a new
problem.
Post by Paul Licameli
Are these three catches complete? I do not yet know. I have not finished
the examination of the code. I have to discover a very large ascending
call graph from NewSimpleBlockFile. I have been writing that in a text
file and it gets pretty big.
I see the need for that exercise. For example I can imagine that the
summary files are a potential additional problem, as if you've failed to
write a block then you shouldn't write the summary file either.

--James.
Paul Licameli
2016-11-19 17:07:47 UTC
Permalink
Attn James
Post by James Crook
Post by Paul Licameli
Post by James Crook
Am a little (but only a little) concerned that we effectively have
::wxMessageBox( e.ErrorMessage() );
in three places. We could end up modifying all three places with later
improvements to reporting. It could be argued that if we are using
exceptions at all for this we should do so 'consistently' and handle and
rethrow as necessary. I'm OK with the current approach though for
2.1.3.
Post by Paul Licameli
What do you mean? The three places mentioned stop the propagation of
error
Post by Paul Licameli
with a recovery. (Actually one of them is not a catch, because of the
problem of communicating the exception inter-thread.)
The concern is it's not DRY.
https://en.wikipedia.org/wiki/Don%27t_repeat_yourself
It's true ::wxMessageBox is only a one liner, and if we later make it a
custom error dialog we will just call the function for that instead.
Nevertheless a part of me is concerned that 'that code to report an
exception to the user' is in three places.
I'm not really repeating myself. I want as few catches as possible but not
fewer. However, that number will be more than three, and they really need
to to different things.

To repeat, the recording problem is inherently different from the others,
because inter-thread communication is needed, and the recovery we want to
do (saving partial results rather than rolling back) is different. It
really needs special code.

I avoided some repetition I might otherwise have had: It took a little bit
of research to figure out that AudacityApp::OnExceptionInMainLoop was a
single high-enough level place to catch errors from dispatch of
CommandManager functors, but also things like track control panel menu
items and presses of Edit toolbar buttons, that don't go through the
command manager. Otherwise I would have written more catch blocks. This
single high level catch is a good thing.

The catch block in BenchmarkDialog::OnRun is just for completeness,
catching a very low probability case that can only affect alpha users who
invoke Audacity from the command line.

But now I think I need more catches, which will do yet other recovery
actions. That is because the exception can happen within callback
functions we write that are called from C libraries such as Nyquist and
various import libraries. I fear such C code is not equipped to handle C++
error propagation through the stack, and the state of the libraries could
be corrupted. The various callback protocols do allow the function to
return an error code instead. So I must add a catch in each callback and
return an appropriate code which varies with the library.

Much code we write are in fact event-handling callbacks too, for wxWidgets,
but I will assume that library is written to tolerate exception propagation
through its stacks. After all they invite you to override
wxApp::OnExceptionInMainLoop.

But now I also fear that this guarantee might not apply to overrides of
wxAccessible methods, which get invoked via other paths (on Windows, in a
method of a COM object; on Mac, with my wxWidgets patches, within layers of
Objective-C). So, I may write more catches.

Or maybe it is enough that in AButton::Click, we call not ProcessEvent but
SafelyProcessEvent so that AudacityApp::OnExceptionInMainLoop has the
opportunity to intercept exceptions for exhaustion of disk space. But now
this makes me wonder whether every one of our uses of ProcessEvent may
likewise need to be replaced with SafelyProcessEvent. Maybe not, because
the topmost event handling loop of the application is still the catch-all
in other cases and there are only layers of our code and wxWidgets code in
between.

CommandManagerEventMonitor::FilterEvent makes an objective-C closure and
that may need to stop exception propagation too.
Post by James Crook
Post by Paul Licameli
Do you worry about all the intermediate levels of stack that get unwound
needing to catch and rethrow? Well no, not if they use RAII properly,
but
Post by Paul Licameli
then there are many such places and that is what I am doubtful of.
It's more an instinct that the ::wxMessageBox should (maybe) be a catch
right at the top.
I think the 'correct' design is that the original exception actually
stops recording NOT the TrackPanel Timer event. The TrackPanel Timer
Event should only be concerned with GUI things, such as making sure the
record button pops up ... and signalling upwards that an error
reporting dialog for the user is needed.
You're signalling the BadWriteException for recording 'upwards' by
testing state, but elsewhere in the GUI code are using an exception to
signal an error in operations upwards. That's why I am talking about
rethrowing - as a way to merge the two approaches. I get that in the
other cases you undo the partial operation, and that that is not right
for the record case. Maybe a flag is needed in the exception that can
be cleared before rethrowing to control that?
It's not that I worry that the code you've written won't work. Rather it
is about separation of concerns - Recording thread stops the recording;
Repainting Timer ensures visuals are right e.g. button pop ups; Top
level catch ensures error is reported to user. Of course we do currently
put more in our GUI thread than we should so it's not as if it's a new
problem.
The asynchronicity is really an inherent difficulty here. The timer in
TrackPanel already polls for the end of playback happening asynchronously
in the other threads, so that it could change button states. Now that
recording might asynchronously stop too, because of disk exhaustion
detected in the worker thread, this seemed a good place to keep all such
polling together.
Post by James Crook
Post by Paul Licameli
Are these three catches complete? I do not yet know. I have not
finished
Post by Paul Licameli
the examination of the code. I have to discover a very large ascending
call graph from NewSimpleBlockFile. I have been writing that in a text
file and it gets pretty big.
I see the need for that exercise. For example I can imagine that the
summary files are a potential additional problem, as if you've failed to
write a block then you shouldn't write the summary file either.
The excercise is still not complete.

This thing is really ballooning -- not the fixes, but the effort to be sure
they are complete. Should I still bother?

It will be good to eliminate a rather low numbered high priority bug, and
it also happens that every occurrency of the comment

// TO DO: Actually handle this.

is in one of the functions that I discovered in the graph. Though perhaps
the comment is not added in every place it ought to be. Yet it means there
is a neglected check of an error code. Perhaps with a little more effort
all such functions could return void and instead throw other subtypes of a
new base class that they will share with BadWriteException. Then, strike
many TO DOs as done.

PRL
Post by James Crook
--James.
------------------------------------------------------------
------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Paul Licameli
2016-11-19 18:26:19 UTC
Permalink
On further reflection:

I reviewed how sound activated recording works. This is done with calls to
CallAfter in AudioIO.cpp, which lets one of the worker threads enqueue a
bit of work to be done in the main thread in idle time.

I like this and can do similar rather than polling in the TrackPanel timer
loop. Best not to junk up TrackPanel.cpp further.

PRL
Post by Paul Licameli
Attn James
Post by James Crook
Post by Paul Licameli
Post by James Crook
Am a little (but only a little) concerned that we effectively have
::wxMessageBox( e.ErrorMessage() );
in three places. We could end up modifying all three places with later
improvements to reporting. It could be argued that if we are using
exceptions at all for this we should do so 'consistently' and handle
and
Post by Paul Licameli
Post by James Crook
rethrow as necessary. I'm OK with the current approach though for
2.1.3.
Post by Paul Licameli
What do you mean? The three places mentioned stop the propagation of
error
Post by Paul Licameli
with a recovery. (Actually one of them is not a catch, because of the
problem of communicating the exception inter-thread.)
The concern is it's not DRY.
https://en.wikipedia.org/wiki/Don%27t_repeat_yourself
It's true ::wxMessageBox is only a one liner, and if we later make it a
custom error dialog we will just call the function for that instead.
Nevertheless a part of me is concerned that 'that code to report an
exception to the user' is in three places.
I'm not really repeating myself. I want as few catches as possible but
not fewer. However, that number will be more than three, and they really
need to to different things.
To repeat, the recording problem is inherently different from the others,
because inter-thread communication is needed, and the recovery we want to
do (saving partial results rather than rolling back) is different. It
really needs special code.
I avoided some repetition I might otherwise have had: It took a little
bit of research to figure out that AudacityApp::OnExceptionInMainLoop was
a single high-enough level place to catch errors from dispatch of
CommandManager functors, but also things like track control panel menu
items and presses of Edit toolbar buttons, that don't go through the
command manager. Otherwise I would have written more catch blocks. This
single high level catch is a good thing.
The catch block in BenchmarkDialog::OnRun is just for completeness,
catching a very low probability case that can only affect alpha users who
invoke Audacity from the command line.
But now I think I need more catches, which will do yet other recovery
actions. That is because the exception can happen within callback
functions we write that are called from C libraries such as Nyquist and
various import libraries. I fear such C code is not equipped to handle C++
error propagation through the stack, and the state of the libraries could
be corrupted. The various callback protocols do allow the function to
return an error code instead. So I must add a catch in each callback and
return an appropriate code which varies with the library.
Much code we write are in fact event-handling callbacks too, for
wxWidgets, but I will assume that library is written to tolerate exception
propagation through its stacks. After all they invite you to override
wxApp::OnExceptionInMainLoop.
But now I also fear that this guarantee might not apply to overrides of
wxAccessible methods, which get invoked via other paths (on Windows, in a
method of a COM object; on Mac, with my wxWidgets patches, within layers of
Objective-C). So, I may write more catches.
Or maybe it is enough that in AButton::Click, we call not ProcessEvent but
SafelyProcessEvent so that AudacityApp::OnExceptionInMainLoop has the
opportunity to intercept exceptions for exhaustion of disk space. But now
this makes me wonder whether every one of our uses of ProcessEvent may
likewise need to be replaced with SafelyProcessEvent. Maybe not, because
the topmost event handling loop of the application is still the catch-all
in other cases and there are only layers of our code and wxWidgets code in
between.
CommandManagerEventMonitor::FilterEvent makes an objective-C closure and
that may need to stop exception propagation too.
Post by James Crook
Post by Paul Licameli
Do you worry about all the intermediate levels of stack that get unwound
needing to catch and rethrow? Well no, not if they use RAII properly,
but
Post by Paul Licameli
then there are many such places and that is what I am doubtful of.
It's more an instinct that the ::wxMessageBox should (maybe) be a catch
right at the top.
I think the 'correct' design is that the original exception actually
stops recording NOT the TrackPanel Timer event. The TrackPanel Timer
Event should only be concerned with GUI things, such as making sure the
record button pops up ... and signalling upwards that an error
reporting dialog for the user is needed.
You're signalling the BadWriteException for recording 'upwards' by
testing state, but elsewhere in the GUI code are using an exception to
signal an error in operations upwards. That's why I am talking about
rethrowing - as a way to merge the two approaches. I get that in the
other cases you undo the partial operation, and that that is not right
for the record case. Maybe a flag is needed in the exception that can
be cleared before rethrowing to control that?
It's not that I worry that the code you've written won't work. Rather it
is about separation of concerns - Recording thread stops the recording;
Repainting Timer ensures visuals are right e.g. button pop ups; Top
level catch ensures error is reported to user. Of course we do currently
put more in our GUI thread than we should so it's not as if it's a new
problem.
The asynchronicity is really an inherent difficulty here. The timer in
TrackPanel already polls for the end of playback happening asynchronously
in the other threads, so that it could change button states. Now that
recording might asynchronously stop too, because of disk exhaustion
detected in the worker thread, this seemed a good place to keep all such
polling together.
Post by James Crook
Post by Paul Licameli
Are these three catches complete? I do not yet know. I have not
finished
Post by Paul Licameli
the examination of the code. I have to discover a very large ascending
call graph from NewSimpleBlockFile. I have been writing that in a text
file and it gets pretty big.
I see the need for that exercise. For example I can imagine that the
summary files are a potential additional problem, as if you've failed to
write a block then you shouldn't write the summary file either.
The excercise is still not complete.
This thing is really ballooning -- not the fixes, but the effort to be
sure they are complete. Should I still bother?
It will be good to eliminate a rather low numbered high priority bug, and
it also happens that every occurrency of the comment
// TO DO: Actually handle this.
is in one of the functions that I discovered in the graph. Though perhaps
the comment is not added in every place it ought to be. Yet it means there
is a neglected check of an error code. Perhaps with a little more effort
all such functions could return void and instead throw other subtypes of a
new base class that they will share with BadWriteException. Then, strike
many TO DOs as done.
PRL
Post by James Crook
--James.
------------------------------------------------------------
------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel
James Crook
2016-11-19 19:11:47 UTC
Permalink
Thanks Paul.

I currently am time poor, and do not have sufficient time to do an
independent analysis/fix.
I do still think we should only need code to invoke displaying a dialog
for out of memory in one place 'at the top' of the GUI. Given your
arguments I would need to do a proof of concept to be convincing, and I
don't currently have time to do that.


There are two parts to terminating record. One is actually stopping
recording. The other is updating the GUI to show it has stopped.
Stopping the actual recording should happen in the recording thread.
Updating the GUI should happen in the GUI thread. Currently in your 437
you signal from the recording thread to the GUI thread that you would
like to stop. The net result is that actual recording stops 'later'
than it should when we run out of disk space. We probably get away with
it, but I contend it isn't necessary to delay stopping the actual
recording. That delay isn't inherent in the problem.


As RM I am looking at the risks of fixing 437 anew. The changes are
more extensive than I had anticipated, and the time to get and test a
fix is also likely to be substantially longer than I anticipated.
Releasing is a feature. I'm therefore going to not block on 437 any
more. We'll have another go at it for 2.1.4. So current plan is to go
into freeze tomorrow. We'll be just over a week in freeze, and then
working to release.

--James.
Post by Paul Licameli
Attn James
Post by James Crook
Post by Paul Licameli
Post by James Crook
Am a little (but only a little) concerned that we effectively have
::wxMessageBox( e.ErrorMessage() );
in three places. We could end up modifying all three places with later
improvements to reporting. It could be argued that if we are using
exceptions at all for this we should do so 'consistently' and handle and
rethrow as necessary. I'm OK with the current approach though for
2.1.3.
Post by Paul Licameli
What do you mean? The three places mentioned stop the propagation of
error
Post by Paul Licameli
with a recovery. (Actually one of them is not a catch, because of the
problem of communicating the exception inter-thread.)
The concern is it's not DRY.
https://en.wikipedia.org/wiki/Don%27t_repeat_yourself
It's true ::wxMessageBox is only a one liner, and if we later make it a
custom error dialog we will just call the function for that instead.
Nevertheless a part of me is concerned that 'that code to report an
exception to the user' is in three places.
I'm not really repeating myself. I want as few catches as possible but not
fewer. However, that number will be more than three, and they really need
to to different things.
To repeat, the recording problem is inherently different from the others,
because inter-thread communication is needed, and the recovery we want to
do (saving partial results rather than rolling back) is different. It
really needs special code.
I avoided some repetition I might otherwise have had: It took a little bit
of research to figure out that AudacityApp::OnExceptionInMainLoop was a
single high-enough level place to catch errors from dispatch of
CommandManager functors, but also things like track control panel menu
items and presses of Edit toolbar buttons, that don't go through the
command manager. Otherwise I would have written more catch blocks. This
single high level catch is a good thing.
The catch block in BenchmarkDialog::OnRun is just for completeness,
catching a very low probability case that can only affect alpha users who
invoke Audacity from the command line.
But now I think I need more catches, which will do yet other recovery
actions. That is because the exception can happen within callback
functions we write that are called from C libraries such as Nyquist and
various import libraries. I fear such C code is not equipped to handle C++
error propagation through the stack, and the state of the libraries could
be corrupted. The various callback protocols do allow the function to
return an error code instead. So I must add a catch in each callback and
return an appropriate code which varies with the library.
Much code we write are in fact event-handling callbacks too, for wxWidgets,
but I will assume that library is written to tolerate exception propagation
through its stacks. After all they invite you to override
wxApp::OnExceptionInMainLoop.
But now I also fear that this guarantee might not apply to overrides of
wxAccessible methods, which get invoked via other paths (on Windows, in a
method of a COM object; on Mac, with my wxWidgets patches, within layers of
Objective-C). So, I may write more catches.
Or maybe it is enough that in AButton::Click, we call not ProcessEvent but
SafelyProcessEvent so that AudacityApp::OnExceptionInMainLoop has the
opportunity to intercept exceptions for exhaustion of disk space. But now
this makes me wonder whether every one of our uses of ProcessEvent may
likewise need to be replaced with SafelyProcessEvent. Maybe not, because
the topmost event handling loop of the application is still the catch-all
in other cases and there are only layers of our code and wxWidgets code in
between.
CommandManagerEventMonitor::FilterEvent makes an objective-C closure and
that may need to stop exception propagation too.
Post by James Crook
Post by Paul Licameli
Do you worry about all the intermediate levels of stack that get unwound
needing to catch and rethrow? Well no, not if they use RAII properly,
but
Post by Paul Licameli
then there are many such places and that is what I am doubtful of.
It's more an instinct that the ::wxMessageBox should (maybe) be a catch
right at the top.
I think the 'correct' design is that the original exception actually
stops recording NOT the TrackPanel Timer event. The TrackPanel Timer
Event should only be concerned with GUI things, such as making sure the
record button pops up ... and signalling upwards that an error
reporting dialog for the user is needed.
You're signalling the BadWriteException for recording 'upwards' by
testing state, but elsewhere in the GUI code are using an exception to
signal an error in operations upwards. That's why I am talking about
rethrowing - as a way to merge the two approaches. I get that in the
other cases you undo the partial operation, and that that is not right
for the record case. Maybe a flag is needed in the exception that can
be cleared before rethrowing to control that?
It's not that I worry that the code you've written won't work. Rather it
is about separation of concerns - Recording thread stops the recording;
Repainting Timer ensures visuals are right e.g. button pop ups; Top
level catch ensures error is reported to user. Of course we do currently
put more in our GUI thread than we should so it's not as if it's a new
problem.
The asynchronicity is really an inherent difficulty here. The timer in
TrackPanel already polls for the end of playback happening asynchronously
in the other threads, so that it could change button states. Now that
recording might asynchronously stop too, because of disk exhaustion
detected in the worker thread, this seemed a good place to keep all such
polling together.
Post by James Crook
Post by Paul Licameli
Are these three catches complete? I do not yet know. I have not
finished
Post by Paul Licameli
the examination of the code. I have to discover a very large ascending
call graph from NewSimpleBlockFile. I have been writing that in a text
file and it gets pretty big.
I see the need for that exercise. For example I can imagine that the
summary files are a potential additional problem, as if you've failed to
write a block then you shouldn't write the summary file either.
The excercise is still not complete.
This thing is really ballooning -- not the fixes, but the effort to be sure
they are complete. Should I still bother?
It will be good to eliminate a rather low numbered high priority bug, and
it also happens that every occurrency of the comment
// TO DO: Actually handle this.
is in one of the functions that I discovered in the graph. Though perhaps
the comment is not added in every place it ought to be. Yet it means there
is a neglected check of an error code. Perhaps with a little more effort
all such functions could return void and instead throw other subtypes of a
new base class that they will share with BadWriteException. Then, strike
many TO DOs as done.
PRL
Post by James Crook
--James.
------------------------------------------------------------
------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel
------------------------------------------------------------------------------
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Loading...