Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1375913005:
Only catch UnsatisfiedLinkError in Logging.java. (Closed)
|
Created:
5 years, 2 months ago by jiayl2 Modified:
5 years, 2 months ago Reviewers:
phoglund CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionOnly catch UnsatisfiedLinkError in Logging.java.
BUG=
Committed: https://crrev.com/4cd053fe88f6b47b3ad7689e86755adc39511b35
Cr-Commit-Position: refs/heads/master@{#10169}
Patch Set 1 #
Total comments: 14
Patch Set 2 : CR #
Total comments: 2
Messages
Total messages: 13 (4 generated)
jiayl@webrtc.org changed reviewers: + phoglund@webrtc.org
PTAL
https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... File webrtc/base/java/src/org/webrtc/Logging.java (right): https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:22: private static boolean tracingEnabled; This class is accessed by several threads rights? In that case make this an AtomicBoolean for thread safety. https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:31: fallbackLogger.log(Level.WARNING, "Failed to load jingle_peerconnection_so: " + t); Log like this instead: fallbackLogger.log(Level.WARNING, "Failed to load jingle_peerconnection_so: ", t); This will print the stack trace and not just t.toString(). https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:64: public static void enableLogThreads() { I would like an assert or RuntimeException here, like if (!libjingleLoaded.get()) throw IllegalStateException("Tried to enable log threads, but the libjingle .so failed to load"); It will fail with an UnsatisfiedLinkError on line 65 today, but the error is really confusing. The above would make it a lot clearer what happened. I assume this code is accessed by several threads, so a thread-safe way to implement this would be to keep a static AtomicBoolean libjingleLoaded in this class, which you set to true in the static initializer if the load succeeds. https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:68: public static void enableLogTimeStamps() { Same here https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:79: int nativeLevel = 0; And here https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:88: if (tracingEnabled) { if (tracingEnabled.get() && libjingleLoaded.get()) { nativeLog(); return; }
PTAL https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... File webrtc/base/java/src/org/webrtc/Logging.java (right): https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:22: private static boolean tracingEnabled; On 2015/10/02 14:27:30, phoglund wrote: > This class is accessed by several threads rights? In that case make this an > AtomicBoolean for thread safety. Added volatile keyword. It's written in a synchronized method. So volatile should be enough. https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:31: fallbackLogger.log(Level.WARNING, "Failed to load jingle_peerconnection_so: " + t); On 2015/10/02 14:27:30, phoglund wrote: > Log like this instead: > > fallbackLogger.log(Level.WARNING, "Failed to load jingle_peerconnection_so: ", > t); > > This will print the stack trace and not just t.toString(). Done. https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:64: public static void enableLogThreads() { On 2015/10/02 14:27:30, phoglund wrote: > I would like an assert or RuntimeException here, like > > if (!libjingleLoaded.get()) > throw IllegalStateException("Tried to enable log threads, but the libjingle > .so failed to load"); > > It will fail with an UnsatisfiedLinkError on line 65 today, but the error is > really confusing. The above would make it a lot clearer what happened. > > I assume this code is accessed by several threads, so a thread-safe way to > implement this would be to keep a static AtomicBoolean libjingleLoaded in this > class, which you set to true in the static initializer if the load succeeds. I added a log instead. Don't want to throw exception because the caller has no way to check if the native lib has loaded. https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:68: public static void enableLogTimeStamps() { On 2015/10/02 14:27:30, phoglund wrote: > Same here Done. https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:79: int nativeLevel = 0; On 2015/10/02 14:27:30, phoglund wrote: > And here Done. https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:88: if (tracingEnabled) { On 2015/10/02 14:27:30, phoglund wrote: > if (tracingEnabled.get() && libjingleLoaded.get()) { > nativeLog(); > return; > } Done. https://codereview.webrtc.org/1375913005/diff/20001/webrtc/base/java/src/org/... File webrtc/base/java/src/org/webrtc/Logging.java (right): https://codereview.webrtc.org/1375913005/diff/20001/webrtc/base/java/src/org/... webrtc/base/java/src/org/webrtc/Logging.java:104: if (tracingEnabled) { Note: tracingEnabled should only be true when native lib has loaded. So no need to check nativeLibLoaded or catch the error.
lgtm https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... File webrtc/base/java/src/org/webrtc/Logging.java (right): https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:22: private static boolean tracingEnabled; On 2015/10/02 15:09:09, jiayl2 wrote: > On 2015/10/02 14:27:30, phoglund wrote: > > This class is accessed by several threads rights? In that case make this an > > AtomicBoolean for thread safety. > > Added volatile keyword. > It's written in a synchronized method. So volatile should be enough. Write or read doesn't matter. In Java, it is _not enough_ to synchronize just where the variable is written. It needs to be synchronized on the read accesses as well, otherwise the code isn't thread-safe! Volatile will suffice in this case. I've always preferred not using volatile but just java.util.concurrent.atomic primitives - this is what's recommended in Java Concurrency in practice. I recommend that but I think volatile is acceptable. https://codereview.webrtc.org/1375913005/diff/1/webrtc/base/java/src/org/webr... webrtc/base/java/src/org/webrtc/Logging.java:64: public static void enableLogThreads() { On 2015/10/02 15:09:09, jiayl2 wrote: > On 2015/10/02 14:27:30, phoglund wrote: > > I would like an assert or RuntimeException here, like > > > > if (!libjingleLoaded.get()) > > throw IllegalStateException("Tried to enable log threads, but the libjingle > > .so failed to load"); > > > > It will fail with an UnsatisfiedLinkError on line 65 today, but the error is > > really confusing. The above would make it a lot clearer what happened. > > > > I assume this code is accessed by several threads, so a thread-safe way to > > implement this would be to keep a static AtomicBoolean libjingleLoaded in this > > class, which you set to true in the static initializer if the load succeeds. > > I added a log instead. Don't want to throw exception because the caller has no > way to check if the native lib has loaded. Depends on how you interpret the contract for enableLogThreads. If the application really depends on getting those threads enabled, I think it's appropriate to throw an exception and crash the app. If the so load failed for PeerConnectionClient, by comparison, it would certainly make sense to crash the app. I can agree it's not worth crashing the app just because logging is broken, but we should write an emulator test that asserts that the load succeeds for Tachyon. https://codereview.webrtc.org/1375913005/diff/20001/webrtc/base/java/src/org/... File webrtc/base/java/src/org/webrtc/Logging.java (right): https://codereview.webrtc.org/1375913005/diff/20001/webrtc/base/java/src/org/... webrtc/base/java/src/org/webrtc/Logging.java:104: if (tracingEnabled) { On 2015/10/02 15:09:09, jiayl2 wrote: > Note: tracingEnabled should only be true when native lib has loaded. So no need > to check nativeLibLoaded or catch the error. Acknowledged.
The CQ bit was checked by jiayl@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375913005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375913005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by jiayl@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375913005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375913005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4cd053fe88f6b47b3ad7689e86755adc39511b35 Cr-Commit-Position: refs/heads/master@{#10169} |
