Uh oh!
There was an error while loading. Please reload this page.
[STORM-2754] Not killing on exceptions in other threads - #2341
Conversation
HeartSaVioR
commented
Sep 22, 2017
Same thought on this: fail-fast if we can't handle the exception. But I'm OK to see others' opinions as well. |
revans2
commented
Sep 22, 2017
@Ethanlm I think we want to combine STORM-2753 with this into a single pull request. I think I need to give a little background on this so it makes since, and I am not sure this is the best solution which is why I would like to see the two combined into a single pull request. We ran into some issues where pacemaker servers were being killed because of a client being shot at the wrong time, or security scans, etc. The issue was that the uncaught exception handler was not what we wanted for the pacemaker server. We then adjusted some things and ended up with an end user that had their topology crashing periodically because an exception that was being thrown on a background thread, and ignored previously, was now fatal. So the goal with these was to find a balance. We want the default uncaught exception handler for all threads to be a little bit more lenient. But for daemon threads we already handle exceptions very strictly so the default UncaughtExceptionHandler is not needed here. But thinking about it now perhaps what we want is something that is a bit more explicit about what is happening. For example we can perhaps have a separate default exception handler for the worker from everything else. |
| public void uncaughtException(Thread thread, Throwable thrown) { | ||
| try { | ||
| handleUncaughtException(thrown); | ||
| handleUncaughtExceptionWithoutKillingProcess(thrown); |
There was a problem hiding this comment.
This will swallow all the Throwables including Errors excluding OOME. Is it intended? We may still want to make it crash when there's Error thrown from somewhere, given that it normally indicates unrecoverable state.
| } | ||
| if(allowedExceptions.contains(t.getClass())) { | ||
| if(alwaysSwallow || allowedExceptions.contains(t.getClass())) { |
There was a problem hiding this comment.
Even alwaysSwallow is set to true, we may still want to throw Error on Errors.
@HeartSaVioR Sorry for the late reply. I made the changes. Could you please review again? This also fixes a potential NPE issue. |
HeartSaVioR
commented
Oct 8, 2017
+1 |
| LOG.error("Exception thrown while handling uncaught exception " + error.getCause()); | ||
| Runtime.getRuntime().exit(1); | ||
| } | ||
| LOG.info("Received error in netty thread.. terminating server..."); |
There was a problem hiding this comment.
If a thread hits the exception handler will it still die? If so who is responsible for replacing it? As far as I can tell this handler is used by a HashedWheelTimer in Context, and it doesn't look to me like that class will replace a dead thread.
There was a problem hiding this comment.
It looks like the HashedWheelTimer will not be replaced - I'll take a second pass at this to confirm
| public void uncaughtException(Thread thread, Throwable thrown) { | ||
| try { | ||
| handleUncaughtException(thrown); | ||
| handleUncaughtExceptionWithoutKillingProcess(thrown); |
There was a problem hiding this comment.
Same question, if a thread gets an exception and the process is not shut down, will the thread be replaced?
We are closing stale Pull Requests to make the list more manageable. Please re-open any Pull Request that has been closed in error. Closesapache#608Closesapache#639Closesapache#640Closesapache#648Closesapache#662Closesapache#668Closesapache#692Closesapache#705Closesapache#724Closesapache#728Closesapache#730Closesapache#753Closesapache#803Closesapache#854Closesapache#922Closesapache#986Closesapache#992Closesapache#1019Closesapache#1040Closesapache#1041Closesapache#1043Closesapache#1046Closesapache#1051Closesapache#1078Closesapache#1146Closesapache#1164Closesapache#1165Closesapache#1178Closesapache#1213Closesapache#1225Closesapache#1258Closesapache#1259Closesapache#1268Closesapache#1272Closesapache#1277Closesapache#1278Closesapache#1288Closesapache#1296Closesapache#1328Closesapache#1342Closesapache#1353Closesapache#1370Closesapache#1376Closesapache#1391Closesapache#1395Closesapache#1399Closesapache#1406Closesapache#1410Closesapache#1422Closesapache#1427Closesapache#1443Closesapache#1462Closesapache#1468Closesapache#1483Closesapache#1506Closesapache#1509Closesapache#1515Closesapache#1520Closesapache#1521Closesapache#1525Closesapache#1527Closesapache#1544Closesapache#1550Closesapache#1566Closesapache#1569Closesapache#1570Closesapache#1575Closesapache#1580Closesapache#1584Closesapache#1591Closesapache#1600Closesapache#1611Closesapache#1613Closesapache#1639Closesapache#1703Closesapache#1711Closesapache#1719Closesapache#1737Closesapache#1760Closesapache#1767Closesapache#1768Closesapache#1785Closesapache#1799Closesapache#1822Closesapache#1824Closesapache#1844Closesapache#1874Closesapache#1918Closesapache#1928Closesapache#1937Closesapache#1942Closesapache#1951Closesapache#1957Closesapache#1963Closesapache#1964Closesapache#1965Closesapache#1967Closesapache#1968Closesapache#1971Closesapache#1985Closesapache#1986Closesapache#1998Closesapache#2031Closesapache#2032Closesapache#2071Closesapache#2076Closesapache#2108Closesapache#2119Closesapache#2128Closesapache#2142Closesapache#2174Closesapache#2206Closesapache#2297Closesapache#2322Closesapache#2332Closesapache#2341Closesapache#2377Closesapache#2414Closesapache#2469
We are closing stale Pull Requests to make the list more manageable. Please re-open any Pull Request that has been closed in error. Closesapache#608Closesapache#639Closesapache#640Closesapache#648Closesapache#662Closesapache#668Closesapache#692Closesapache#705Closesapache#724Closesapache#728Closesapache#730Closesapache#753Closesapache#803Closesapache#854Closesapache#922Closesapache#986Closesapache#992Closesapache#1019Closesapache#1040Closesapache#1041Closesapache#1043Closesapache#1046Closesapache#1051Closesapache#1078Closesapache#1146Closesapache#1164Closesapache#1165Closesapache#1178Closesapache#1213Closesapache#1225Closesapache#1258Closesapache#1259Closesapache#1268Closesapache#1272Closesapache#1277Closesapache#1278Closesapache#1288Closesapache#1296Closesapache#1328Closesapache#1342Closesapache#1353Closesapache#1370Closesapache#1376Closesapache#1391Closesapache#1395Closesapache#1399Closesapache#1406Closesapache#1410Closesapache#1422Closesapache#1427Closesapache#1443Closesapache#1462Closesapache#1468Closesapache#1483Closesapache#1506Closesapache#1509Closesapache#1515Closesapache#1520Closesapache#1521Closesapache#1525Closesapache#1527Closesapache#1544Closesapache#1550Closesapache#1566Closesapache#1569Closesapache#1570Closesapache#1575Closesapache#1580Closesapache#1584Closesapache#1591Closesapache#1600Closesapache#1611Closesapache#1613Closesapache#1639Closesapache#1703Closesapache#1711Closesapache#1719Closesapache#1737Closesapache#1760Closesapache#1767Closesapache#1768Closesapache#1785Closesapache#1799Closesapache#1822Closesapache#1824Closesapache#1844Closesapache#1874Closesapache#1918Closesapache#1928Closesapache#1937Closesapache#1942Closesapache#1951Closesapache#1957Closesapache#1963Closesapache#1964Closesapache#1965Closesapache#1967Closesapache#1968Closesapache#1971Closesapache#1985Closesapache#1986Closesapache#1998Closesapache#2031Closesapache#2032Closesapache#2071Closesapache#2076Closesapache#2108Closesapache#2119Closesapache#2128Closesapache#2142Closesapache#2174Closesapache#2206Closesapache#2297Closesapache#2322Closesapache#2332Closesapache#2341Closesapache#2377Closesapache#2414Closesapache#2469
JIRA-STORM-2754
We probably don't want to kill the process if the exceptions are from other threads