zeppelin-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From prabhjyotsi...@apache.org
Subject zeppelin git commit: ZEPPELIN-3112: Markdown interpreter fails with NPE
Date Fri, 22 Dec 2017 07:10:58 GMT
Repository: zeppelin
Updated Branches:
  refs/heads/master 6caf587e1 -> 5d09a7f83


ZEPPELIN-3112: Markdown interpreter fails with NPE

### What is this PR for?
Since pegdown-parser is not thread-safe while trying to run multiple MarkDown paragraphs at
once, sometimes it fails to render HTML.
Ref: https://github.com/sirthias/pegdown/blob/master/src/main/java/org/pegdown/PegDownProcessor.java#L32

### What type of PR is it?
[Improvement]

### What is the Jira issue?
* [ZEPPELIN-3112](https://issues.apache.org/jira/browse/ZEPPELIN-3112)

### How should this be tested?
* This happens rarely, when you try to run all paragraph from UI which has more the 5-6 `%md`
paragraph. This is hard to reproduce in 0.8.0, but can easily be done via 0.7.3. Also, have
added a sample [notebook](https://issues.apache.org/jira/secure/attachment/12903037/Test%20MD%20fail.json)
in the parent JIRA

* Have added test case to verify.

### Questions:
* Does the licenses files need update? N/A
* Is there breaking changes for older versions? N/A
* Does this needs documentation? N/A

Author: Prabhjyot Singh <prabhjyotsingh@gmail.com>

Closes #2711 from prabhjyotsingh/ZEPPELIN-3112 and squashes the following commits:

e796e52cd [Prabhjyot Singh] ZEPPELIN-3112: call markdownToHtml in synchronized block


Project: http://git-wip-us.apache.org/repos/asf/zeppelin/repo
Commit: http://git-wip-us.apache.org/repos/asf/zeppelin/commit/5d09a7f8
Tree: http://git-wip-us.apache.org/repos/asf/zeppelin/tree/5d09a7f8
Diff: http://git-wip-us.apache.org/repos/asf/zeppelin/diff/5d09a7f8

Branch: refs/heads/master
Commit: 5d09a7f836ebaddd40e644ef82d80320570364aa
Parents: 6caf587
Author: Prabhjyot Singh <prabhjyotsingh@gmail.com>
Authored: Wed Dec 20 18:09:04 2017 +0530
Committer: Prabhjyot Singh <prabhjyotsingh@gmail.com>
Committed: Fri Dec 22 12:40:53 2017 +0530

----------------------------------------------------------------------
 .../apache/zeppelin/markdown/PegdownParser.java |  6 ++-
 .../zeppelin/markdown/PegdownParserTest.java    | 39 ++++++++++++++++++--
 2 files changed, 39 insertions(+), 6 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/zeppelin/blob/5d09a7f8/markdown/src/main/java/org/apache/zeppelin/markdown/PegdownParser.java
----------------------------------------------------------------------
diff --git a/markdown/src/main/java/org/apache/zeppelin/markdown/PegdownParser.java b/markdown/src/main/java/org/apache/zeppelin/markdown/PegdownParser.java
index baf18f0..fb99f05 100644
--- a/markdown/src/main/java/org/apache/zeppelin/markdown/PegdownParser.java
+++ b/markdown/src/main/java/org/apache/zeppelin/markdown/PegdownParser.java
@@ -41,8 +41,10 @@ public class PegdownParser implements MarkdownParser {
   @Override
   public String render(String markdownText) {
     String html = "";
-    String parsed = processor.markdownToHtml(markdownText);
-
+    String parsed;
+    synchronized (processor) {
+      parsed = processor.markdownToHtml(markdownText);
+    }
     if (null == parsed) {
       throw new RuntimeException("Cannot parse markdown text to HTML using pegdown");
     }

http://git-wip-us.apache.org/repos/asf/zeppelin/blob/5d09a7f8/markdown/src/test/java/org/apache/zeppelin/markdown/PegdownParserTest.java
----------------------------------------------------------------------
diff --git a/markdown/src/test/java/org/apache/zeppelin/markdown/PegdownParserTest.java b/markdown/src/test/java/org/apache/zeppelin/markdown/PegdownParserTest.java
index 0c545dc..2e1d857 100644
--- a/markdown/src/test/java/org/apache/zeppelin/markdown/PegdownParserTest.java
+++ b/markdown/src/test/java/org/apache/zeppelin/markdown/PegdownParserTest.java
@@ -19,6 +19,7 @@ package org.apache.zeppelin.markdown;
 
 import static org.junit.Assert.assertEquals;
 
+import java.util.ArrayList;
 import java.util.Properties;
 import org.apache.zeppelin.interpreter.InterpreterResult;
 
@@ -26,10 +27,8 @@ import static org.apache.zeppelin.markdown.PegdownParser.wrapWithMarkdownClassDi
 import static org.junit.Assert.assertThat;
 
 import org.hamcrest.CoreMatchers;
-import org.junit.After;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Test;
+import org.junit.*;
+import org.junit.rules.ErrorCollector;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -37,6 +36,9 @@ public class PegdownParserTest {
   Logger logger = LoggerFactory.getLogger(PegdownParserTest.class);
   Markdown md;
 
+  @Rule
+  public ErrorCollector collector = new ErrorCollector();
+
   @Before
   public void setUp() throws Exception {
     Properties props = new Properties();
@@ -51,6 +53,35 @@ public class PegdownParserTest {
   }
 
   @Test
+  public void testMultipleThread() {
+    ArrayList<Thread> arrThreads = new ArrayList<Thread>();
+    for (int i = 0; i < 10; i++) {
+      Thread t = new Thread() {
+        public void run() {
+          String r1 = null;
+          try {
+            r1 = md.interpret("# H1", null).code().name();
+          } catch (Exception e) {
+            logger.error("testTestMultipleThread failed to interpret", e);
+          }
+          collector.checkThat("SUCCESS",
+              CoreMatchers.containsString(r1));
+        }
+      };
+      t.start();
+      arrThreads.add(t);
+    }
+
+    for (int i = 0; i < 10; i++) {
+      try {
+        arrThreads.get(i).join();
+      } catch (InterruptedException e) {
+        logger.error("testTestMultipleThread failed to join threads", e);
+      }
+    }
+  }
+
+  @Test
   public void testHeader() {
     InterpreterResult r1 = md.interpret("# H1", null);
     assertEquals(wrapWithMarkdownClassDiv("<h1>H1</h1>"), r1.message().get(0).getData());


Mime
View raw message