GNOME Bugzilla – Bug 302253
Gnopernicus should attempt to recover from gnome-speech errors/failures
Last modified: 2005-05-04 10:24:07 UTC
If gnome-speech crashes, or the communication between gnopernicus and gnome-speech is interrupted somehow, gnopernicus should try to restart the speech module. I will attach a patch that un-sets and then sets the "enable-speech" gconf key in case of an error.
Created attachment 45771 [details] [review] proposed patch
Comment on attachment 45771 [details] [review] proposed patch I have two concerns about this patch: (1) how can we be sure this won't generate an infinite loop in cases where speech calls always result in CORBA errors? It seems to me that because we check gnome-speech errors during the speech init process itself, we could loop forever trying to start a speech driver that's broken. We should have some way of 'giving up' if the speech init process itself fails. (2) a more minor concern, it feels a bit like a hack to unset/reset the gconf key for use_speech; shouldn't we have a different way to restart speech in an idle handler?
Comment on attachment 45771 [details] [review] proposed patch Hi Bill, I think below is the answer to your concern number 1. >+ if (srs_gs_wrap_reset_callback && !reset_already_called) >+ { >+ reset_already_called = TRUE; >+ srs_gs_wrap_reset_callback (); >+ } This is where the reset funtion is called. notice that it's only called if it wasn't called before and if the callback is not NULL. Below is a part of the _init function : the callback is set only if the init function returns TRUE (gnome-speech is initialized successfully) otherwise it will be NULL so it will never be called. >+ if (rv) >+ { >+ reset_already_called = FALSE; /* other functions may give errors while resetting*/ >+ srs_gs_wrap_reset_callback = reset_callback; > } > return rv; > } For concern no 2: Resetting the speech in another way than gconf is very hard because of reentrancy problems and lots of code would have to be added. Using the gconf keys, is a lot easyer because all of these problems are avoided. Besides, this is done when the user starts/stops the speech from the UI.
" Besides, this is done when the user starts/stops the speech from the UI." Yes, but that's not the same as gnopernicus restarting the service due to error. It still feels like a hack to me.
Definitely a hack, but I take Dragan at his word that lots of code would have to be added to do it differently/correctly. The patch is fairly small, and seems reasonable to me. If I'm reading the code correctly, if there is a CORBA error in speech the code waits one second and then restarts the gnome-speech connection. This seems reasonable to me. But... I don't see where reset_already_called gets reset to FALSE though - is srs_gs_wrap_init() called on every gnome-speech call? Or is it that we go to TRUE and then call SET_SRCORE_CONFIG_DATA() which winds up calling srs_gs_wrap_init() which in turn resets reset_already_called to FALSE? If that's the case, then I am OK with this patch going in (assuming it tests out OK). I like that you print an error message to stderr; I'd like to be able to track the fact that speech is resetting... I would like to see the code cleaned up in the future (version 1.1?) so that you don't have to go through gconf to get this done. Bill - at this point do you object to this code going in?
Comment on attachment 45771 [details] [review] proposed patch Patch committed on cvs head.