Bugzilla – Bug 9613
Implicit refresh should raise InvalidDepotResponseException
Last modified: 2009-07-01 16:40:00 UTC
You need to log in before you can comment on or make changes to this bug.
There are a number of places in the API where it's possible to find the following code: if refresh_catalogs: try: self.img.refresh_publishers( progtrack=self.progresstracker) except KeyboardInterrupt: raise except: # Since this is not a refresh # that was explicitly requested, # it doesn't matter if it fails. pass While it may not matter if the refresh fails, passing all exceptions aside from KeyboardInterrupt is unwise. If the underlying code returns an error that indicates the client is misconfigured, we're going to drive on instead of failing early. This can lead to the client hanging around and attempting to do work, when it can get none done. We should change this to explicitly raise the configuration errors we must catch, and pass on the subset of errors we expect not to care about. Changing this to pass on TransportError and raise InvalidDepotResponseException should be sufficient
See also bug 9508
This was intentional with the logic that if the implicit refresh failed, then a better failure reason would happen later, especially if it was due to client misconfiguration. There are more possibly failure cases here that we don't care about than just TransportError (at least for this specific case). For example, the refresh could have failed because the user was attempting an install -nv and they didn't have permission to write the catalog data out. So, if the simple argument is just that we should always explicitly name all of the various types of errors we want to ignore, that is fine. But in this particular case, it was intentional that we ignore all errors, even configuration ones, that are related to a refresh failure.
(In reply to comment #2) > There are more possibly failure cases here that we don't care about than just > TransportError (at least for this specific case). <...> > So, if the simple argument is just that we should always explicitly name all of > the various types of errors we want to ignore, that is fine. But in this > particular case, it was intentional that we ignore all errors, even > configuration ones, that are related to a refresh failure. Ok. Empirical experience shows that catching all exceptions here has unintended consequences; however, I'll switch to a proposed solution that raises the InvalidDepotResponseException and catches all others. However, I suspect that this may cost another engineer debugging time, just as it caught me unable to figure out where my exception had disappeared.
(In reply to comment #3) > (In reply to comment #2) > > There are more possibly failure cases here that we don't care about than just > > TransportError (at least for this specific case). > <...> > > So, if the simple argument is just that we should always explicitly name all of > > the various types of errors we want to ignore, that is fine. But in this > > particular case, it was intentional that we ignore all errors, even > > configuration ones, that are related to a refresh failure. > > Ok. Empirical experience shows that catching all exceptions here has > unintended consequences; however, I'll switch to a proposed solution that > raises the InvalidDepotResponseException and catches all others. However, I > suspect that this may cost another engineer debugging time, just as it caught > me unable to figure out where my exception had disappeared. I'm not saying that what is there is right. I agree that taking the Code Sledge Hammer(TM) to the exception case may be undesirable. Obviously, one annoying thing about it is that it could always fail in this case and we'd never know. So, again, I have no problem with explicit enumeration of the expected failure cases. So, I'm not certain what the right answer is, but I do know that this particular exception case was very intentional and resolved several permissions-related failures among others.
This bug is being fixed as part of the transport re-design. A preliminary webrev is available from: http://cr.opensolaris.org/~johansen/webrev-xport-1/
Integrated 1Jul2009 as change set a48bee2a4b2e9c8345c29acea63116acf77dddb3