Uploaded image for project: 'infrautils'
  1. infrautils
  2. INFRAUTILS-51

SystemReadyListener onSystemBootReady() should allow throws Throwable

XMLWordPrintable

    • Icon: Improvement Improvement
    • Resolution: Done
    • Icon: Medium Medium
    • Neon
    • None
    • None
    • None

      while code reviewing SridharG's https://git.opendaylight.org/gerrit/#/c/76325/ for exception handling correctness, I followed the trail from its MBeanUtils.constructJmxUrl() "upwards" to its caller, finding it's used in DiagStatusServiceMBeanImpl's SystemReadyListener onSystemBootReady() which currently looks like this:

      try {
          jmxConnector = MBeanUtils.startRMIConnectorServer(mbeanServer, host);
      } catch (IOException e) {
          LOG.error("unable to start jmx connector for host: {}", host, e);
      }

      This is somewhat problematic because in the case of what c/76325 fixes, MBeanUtils.startRMIConnectorServer fails with a RuntimeException not an IOException.

      Everyone typically gets exception handling wrong. I think SystemReadyListener onSystemBootReady() is one of those cases where it would make sense to have it declare "throws Throwable" (or Exception, if anyone has any very strong preference that way). This would signal to any implementors of onSystemBootReady() that they can just propagate anything that went wrong, instead of catching and logging but swallowing. That way we can handle it correctly in SystemReadyImpl - by setting the system boot state to failure (because if a SystemReadyListener failed, then things are NOT fine, and we should not swallow that).

      SystemReadyImpl already catches RuntimeException from any SystemReadyListener onSystemBootReady, this change will just let it catch any Throwable/Exception, which IMHO is better for error handling overall.

            vorburger Michael Vorburger
            vorburger Michael Vorburger
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated:
              Resolved: