From e6e9073fc790009f4f7c1b31e5cfea1474972ecc Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Sat, 15 Sep 2018 02:35:03 +0200 Subject: [PATCH 1/5] Fix IOException when LockToken#close fails This happened if the LockTokens hard link was already deleted earlier. Bug: 531759 Change-Id: Idc84bd695fac1a763b3cbb797c9c4c636a16e329 Signed-off-by: Matthias Sohn --- .../src/org/eclipse/jgit/util/FS.java | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java index 6b537a477..8d4e5e52a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java @@ -841,13 +841,18 @@ public boolean isCreated() { @Override public void close() { - if (link.isPresent()) { - try { - Files.delete(link.get()); - } catch (IOException e) { - LOG.error(MessageFormat.format(JGitText.get().closeLockTokenFailed, - this), e); - } + if (!link.isPresent()) { + return; + } + Path p = link.get(); + if (!Files.exists(p)) { + return; + } + try { + Files.delete(p); + } catch (IOException e) { + LOG.error(MessageFormat + .format(JGitText.get().closeLockTokenFailed, this), e); } } From f8e514c74a039209488ef56948c784c00a1d87b3 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Sat, 15 Sep 2018 21:09:51 +0200 Subject: [PATCH 2/5] ObjectDownloadListener: Return from onWritePossible when data is written MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When buffer was written not only call AsyncContext#complete() but also return from the ObjectDownloadListener#onWritePossible(). This avoids endless loop after upgrading from Jetty 9.3.x to 9.4.x lines. In Jetty example implementation:[1] the return statemnt is also used: // If we are at EOF then complete   if (len < 0)   {    async.complete();     return;   } See also this issue upstream: [2]. [1] https://webtide.com/servlet-3-1-async-io-and-jetty [2] https://github.com/eclipse/jetty.project/issues/2911 Change-Id: Iac73fb25e67d40228a378a8e34103f1d28b72a76 Signed-off-by: David Ostrovsky --- .../org/eclipse/jgit/lfs/server/fs/ObjectDownloadListener.java | 1 + 1 file changed, 1 insertion(+) diff --git a/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/ObjectDownloadListener.java b/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/ObjectDownloadListener.java index cc4350090..0b9642642 100644 --- a/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/ObjectDownloadListener.java +++ b/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/ObjectDownloadListener.java @@ -127,6 +127,7 @@ public void onWritePossible() throws IOException { outChannel.write(buffer); } else { context.complete(); + return; } } } From 5c134f4d42e12e9192f2beca8370c1b8bd58c08d Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Sat, 15 Sep 2018 21:09:51 +0200 Subject: [PATCH 3/5] ObjectDownloadListener#onWritePossible: Make code spec compatible Current code violates the ServletOutputStream contract. For every out.isReady() == true either write or close of that ServletOutputStream should be called. See also this issue upstream for more context: [1]. [1] https://github.com/eclipse/jetty.project/issues/2911 Change-Id: Ied575f3603a6be0d2dafc6c3329d685fc212c7a3 Signed-off-by: David Ostrovsky Signed-off-by: Matthias Sohn --- .../lfs/server/fs/ObjectDownloadListener.java | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/ObjectDownloadListener.java b/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/ObjectDownloadListener.java index 0b9642642..60258602f 100644 --- a/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/ObjectDownloadListener.java +++ b/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/ObjectDownloadListener.java @@ -80,7 +80,7 @@ public class ObjectDownloadListener implements WriteListener { private final WritableByteChannel outChannel; - private final ByteBuffer buffer = ByteBuffer.allocateDirect(8192); + private ByteBuffer buffer = ByteBuffer.allocateDirect(8192); /** * @param repository @@ -115,20 +115,26 @@ public ObjectDownloadListener(FileLfsRepository repository, @Override public void onWritePossible() throws IOException { while (out.isReady()) { - if (in.read(buffer) != -1) { - buffer.flip(); - outChannel.write(buffer); - buffer.compact(); - } else { - in.close(); - buffer.flip(); - while (out.isReady()) { - if (buffer.hasRemaining()) { - outChannel.write(buffer); - } else { + try { + buffer.clear(); + if (in.read(buffer) < 0) { + buffer = null; + } else { + buffer.flip(); + } + } catch(Throwable t) { + LOG.log(Level.SEVERE, t.getMessage(), t); + buffer = null; + } finally { + if (buffer != null) { + outChannel.write(buffer); + } else { + try { + out.close(); + } finally { context.complete(); - return; } + return; } } } From c18c768678094dba36e4d394de7a673d1a8764c4 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Mon, 17 Sep 2018 00:35:33 +0200 Subject: [PATCH 4/5] Fix error handling in FileLfsServlet Check in #sendError method if the response was committed already. If yes we cannot set response status or send an error message, last resort is to close the outputstream. If the response wasn't yet committed first reset the response before using writer to send the error message to the client since mixing STREAM and WRITE mode (mixing asynchronous and blocking I/O) is illegal in servlet 3.1. see the following bugs in the gerrit and jetty issue trackers https://bugs.chromium.org/p/gerrit/issues/detail?id=9667 https://bugs.chromium.org/p/gerrit/issues/detail?id=9721 https://github.com/eclipse/jetty.project/issues/2911 Change-Id: Ie35563c2e0ac1c5e918185a746622589a880dc7f Signed-off-by: Matthias Sohn --- .../src/org/eclipse/jgit/lfs/server/fs/FileLfsServlet.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/FileLfsServlet.java b/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/FileLfsServlet.java index a8e3c11e2..15c4448da 100644 --- a/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/FileLfsServlet.java +++ b/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/FileLfsServlet.java @@ -202,6 +202,11 @@ static class Error { */ protected static void sendError(HttpServletResponse rsp, int status, String message) throws IOException { + if (rsp.isCommitted()) { + rsp.getOutputStream().close(); + return; + } + rsp.reset(); rsp.setStatus(status); PrintWriter writer = rsp.getWriter(); gson.toJson(new Error(message), writer); From 1a4e12a451217075310458b94a39bfc132abb276 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Tue, 18 Sep 2018 01:14:34 +0200 Subject: [PATCH 5/5] Fix ObjectUploadListener#close Do not try to set response status if response is already committed. Change-Id: I9a7c2871c86eb53416b905324775f3ed961c8ae6 Signed-off-by: Matthias Sohn --- .../org/eclipse/jgit/lfs/server/fs/ObjectUploadListener.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/ObjectUploadListener.java b/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/ObjectUploadListener.java index 84e4e6f1c..da8688047 100644 --- a/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/ObjectUploadListener.java +++ b/org.eclipse.jgit.lfs.server/src/org/eclipse/jgit/lfs/server/fs/ObjectUploadListener.java @@ -150,7 +150,9 @@ protected void close() throws IOException { channel.close(); // TODO check if status 200 is ok for PUT request, HTTP foresees 204 // for successful PUT without response body - response.setStatus(HttpServletResponse.SC_OK); + if (!response.isCommitted()) { + response.setStatus(HttpServletResponse.SC_OK); + } } finally { context.complete(); }