From 63bd24cf356af4a354ef452b01411d1f2cbbec80 Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Tue, 23 Jul 2019 11:10:24 -0700 Subject: [PATCH 1/3] UploadPack: Introduce ErrorWriter ErrorWriter writes an error message to the user. The implementation is swapped once it detects that the client supports sideband. By default it uses the protocol level ERR packet, which was introduced recently. In total the error output is done in two different places; UploadPack#upload and UploadPack#sendPack. These will be consolidated in the next change. Change-Id: Ia8d72e31170bbeafc8ffa8ddb92702196af8a587 Signed-off-by: Masaya Suzuki --- .../eclipse/jgit/transport/UploadPack.java | 52 +++++++++++++------ 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index 20b45882d..b664cfe7b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -44,6 +44,7 @@ package org.eclipse.jgit.transport; import static java.util.Collections.unmodifiableMap; +import static java.util.Objects.requireNonNull; import static org.eclipse.jgit.lib.Constants.R_TAGS; import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_REF_IN_WANT; import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_SERVER_OPTION; @@ -282,6 +283,8 @@ private static interface IOConsumer { private OutputStream msgOut = NullOutputStream.INSTANCE; + private ErrorWriter errOut = new PackProtocolErrorWriter(); + /** * Refs eligible for advertising to the client, set using * {@link #setAdvertisedRefs}. @@ -812,9 +815,9 @@ public void upload(InputStream input, OutputStream output, // nothing. throw err; } catch (ServiceMayNotContinueException err) { - if (!err.isOutput() && err.getMessage() != null && pckOut != null) { + if (!err.isOutput() && err.getMessage() != null) { try { - pckOut.writeString("ERR " + err.getMessage() + "\n"); //$NON-NLS-1$ //$NON-NLS-2$ + errOut.writeError(err.getMessage()); } catch (IOException e) { err.addSuppressed(e); throw err; @@ -823,12 +826,12 @@ public void upload(InputStream input, OutputStream output, } throw err; } catch (IOException | RuntimeException | Error err) { - if (pckOut != null) { + if (rawOut != null) { String msg = err instanceof PackProtocolException ? err.getMessage() : JGitText.get().internalServerError; try { - pckOut.writeString("ERR " + msg + "\n"); //$NON-NLS-1$ //$NON-NLS-2$ + errOut.writeError(msg); } catch (IOException e) { err.addSuppressed(e); throw err; @@ -2115,6 +2118,8 @@ private void sendPack(PackStatistics.Accumulator accumulator, boolean sideband = caps.contains(OPTION_SIDE_BAND) || caps.contains(OPTION_SIDE_BAND_64K); if (sideband) { + errOut = new SideBandErrorWriter(); + try { sendPack(true, req, accumulator, allTags, unshallowCommits, deepenNots, pckOut); @@ -2124,7 +2129,7 @@ private void sendPack(PackStatistics.Accumulator accumulator, message = JGitText.get().internalServerError; } try { - reportInternalServerErrorOverSideband(message); + errOut.writeError(message); } catch (IOException e) { err.addSuppressed(e); throw err; @@ -2132,8 +2137,7 @@ private void sendPack(PackStatistics.Accumulator accumulator, throw new UploadPackInternalServerErrorException(err); } catch (IOException | RuntimeException | Error err) { try { - reportInternalServerErrorOverSideband( - JGitText.get().internalServerError); + errOut.writeError(JGitText.get().internalServerError); } catch (IOException e) { err.addSuppressed(e); throw err; @@ -2146,16 +2150,6 @@ private void sendPack(PackStatistics.Accumulator accumulator, } } - private void reportInternalServerErrorOverSideband(String message) - throws IOException { - @SuppressWarnings("resource" /* java 7 */) - SideBandOutputStream err = new SideBandOutputStream( - SideBandOutputStream.CH_ERROR, SideBandOutputStream.SMALL_BUF, - rawOut); - err.write(Constants.encode(message)); - err.flush(); - } - /** * Send the requested objects to the client. * @@ -2411,4 +2405,28 @@ void stopBuffering() throws IOException { } } } + + private interface ErrorWriter { + void writeError(String message) throws IOException; + } + + private class SideBandErrorWriter implements ErrorWriter { + @Override + public void writeError(String message) throws IOException { + @SuppressWarnings("resource" /* java 7 */) + SideBandOutputStream err = new SideBandOutputStream( + SideBandOutputStream.CH_ERROR, + SideBandOutputStream.SMALL_BUF, requireNonNull(rawOut)); + err.write(Constants.encode(message)); + err.flush(); + } + } + + private class PackProtocolErrorWriter implements ErrorWriter { + @Override + public void writeError(String message) throws IOException { + new PacketLineOut(requireNonNull(rawOut)) + .writeString("ERR " + message + '\n'); //$NON-NLS-1$ + } + } } From 1e3a7bcef7902e4f035441d878ee056345883197 Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Tue, 23 Jul 2019 10:36:14 -0700 Subject: [PATCH 2/3] UploadPack: Consolidate the sideband handling code to one place This consolidates the sideband stream creation code and the error handling code for the sideband-allowed part in the Git protocol to one place. Change-Id: I0e3e94564f50d1be32006f9d8bcd1ef1ce6bf07e Signed-off-by: Masaya Suzuki --- .../eclipse/jgit/transport/UploadPack.java | 90 ++++++------------- 1 file changed, 29 insertions(+), 61 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index b664cfe7b..8d725b0b5 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -809,11 +809,6 @@ public void upload(InputStream input, OutputStream output, } else { service(pckOut); } - } catch (UploadPackInternalServerErrorException err) { - // UploadPackInternalServerErrorException is a special exception - // that indicates an error is already written to the client. Do - // nothing. - throw err; } catch (ServiceMayNotContinueException err) { if (!err.isOutput() && err.getMessage() != null) { try { @@ -2117,45 +2112,42 @@ private void sendPack(PackStatistics.Accumulator accumulator, Set caps = req.getClientCapabilities(); boolean sideband = caps.contains(OPTION_SIDE_BAND) || caps.contains(OPTION_SIDE_BAND_64K); + if (sideband) { errOut = new SideBandErrorWriter(); - try { - sendPack(true, req, accumulator, allTags, unshallowCommits, - deepenNots, pckOut); - } catch (ServiceMayNotContinueException err) { - String message = err.getMessage(); - if (message == null) { - message = JGitText.get().internalServerError; - } - try { - errOut.writeError(message); - } catch (IOException e) { - err.addSuppressed(e); - throw err; - } - throw new UploadPackInternalServerErrorException(err); - } catch (IOException | RuntimeException | Error err) { - try { - errOut.writeError(JGitText.get().internalServerError); - } catch (IOException e) { - err.addSuppressed(e); - throw err; - } - throw new UploadPackInternalServerErrorException(err); + int bufsz = SideBandOutputStream.SMALL_BUF; + if (req.getClientCapabilities().contains(OPTION_SIDE_BAND_64K)) { + bufsz = SideBandOutputStream.MAX_BUF; } + OutputStream packOut = new SideBandOutputStream( + SideBandOutputStream.CH_DATA, bufsz, rawOut); + + ProgressMonitor pm = NullProgressMonitor.INSTANCE; + if (!req.getClientCapabilities().contains(OPTION_NO_PROGRESS)) { + msgOut = new SideBandOutputStream( + SideBandOutputStream.CH_PROGRESS, bufsz, rawOut); + pm = new SideBandProgressMonitor(msgOut); + } + + sendPack(pm, pckOut, packOut, req, accumulator, allTags, + unshallowCommits, deepenNots); + pckOut.end(); } else { - sendPack(false, req, accumulator, allTags, unshallowCommits, deepenNots, - pckOut); + sendPack(NullProgressMonitor.INSTANCE, pckOut, rawOut, req, + accumulator, allTags, unshallowCommits, deepenNots); } } /** * Send the requested objects to the client. * - * @param sideband - * whether to wrap the pack in side-band pkt-lines, interleaved - * with progress messages and errors. + * @param pm + * progress monitor + * @param pckOut + * PacketLineOut that shares the output with packOut + * @param packOut + * packfile output * @param req * request being processed * @param accumulator @@ -2167,35 +2159,14 @@ private void sendPack(PackStatistics.Accumulator accumulator, * shallow commits on the client that are now becoming unshallow * @param deepenNots * objects that the client specified using --shallow-exclude - * @param pckOut - * output writer * @throws IOException * if an error occurred while generating or writing the pack. */ - private void sendPack(final boolean sideband, - FetchRequest req, + private void sendPack(ProgressMonitor pm, PacketLineOut pckOut, + OutputStream packOut, FetchRequest req, PackStatistics.Accumulator accumulator, - @Nullable Collection allTags, - List unshallowCommits, - List deepenNots, - PacketLineOut pckOut) throws IOException { - ProgressMonitor pm = NullProgressMonitor.INSTANCE; - OutputStream packOut = rawOut; - - if (sideband) { - int bufsz = SideBandOutputStream.SMALL_BUF; - if (req.getClientCapabilities().contains(OPTION_SIDE_BAND_64K)) - bufsz = SideBandOutputStream.MAX_BUF; - - packOut = new SideBandOutputStream(SideBandOutputStream.CH_DATA, - bufsz, rawOut); - if (!req.getClientCapabilities().contains(OPTION_NO_PROGRESS)) { - msgOut = new SideBandOutputStream( - SideBandOutputStream.CH_PROGRESS, bufsz, rawOut); - pm = new SideBandProgressMonitor(msgOut); - } - } - + @Nullable Collection allTags, List unshallowCommits, + List deepenNots) throws IOException { if (wantAll.isEmpty()) { preUploadHook.onSendPack(this, wantIds, commonBase); } else { @@ -2338,9 +2309,6 @@ else if (ref.getName().startsWith(Constants.R_HEADS)) } pw.close(); } - - if (sideband) - pckOut.end(); } private static void findSymrefs( From b8d9734c0268446cddac281ec762808ac369538e Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Tue, 6 Aug 2019 11:12:43 -0700 Subject: [PATCH 3/3] UploadPack: Create a method that propagates an exception as-is Exception handling can be isolated from UploadPack. This makes it possible to make the exception handler pluggable. Change-Id: Ieebbd6711963c7f2e47a98783b4ad815793721c7 Signed-off-by: Masaya Suzuki --- .../eclipse/jgit/transport/UploadPack.java | 112 +++++++++++------- 1 file changed, 71 insertions(+), 41 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index 8d725b0b5..9c4e000c7 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -759,56 +759,25 @@ private boolean useProtocolV2() { /** * Execute the upload task on the socket. * - *

If the client passed extra parameters (e.g., "version=2") through a - * side channel, the caller must call setExtraParameters first to supply - * them. + *

+ * Same as {@link #uploadWithExceptionPropagation} except that the thrown + * exceptions are handled in the method, and the error messages are sent to + * the clients. + * + *

+ * Call this method if the caller does not have an error handling mechanism. + * Call {@link #uploadWithExceptionPropagation} if the caller wants to have + * its own error handling mechanism. * * @param input - * raw input to read client commands from. Caller must ensure the - * input is buffered, otherwise read performance may suffer. * @param output - * response back to the Git network client, to write the pack - * data onto. Caller must ensure the output is buffered, - * otherwise write performance may suffer. * @param messages - * secondary "notice" channel to send additional messages out - * through. When run over SSH this should be tied back to the - * standard error channel of the command execution. For most - * other network connections this should be null. * @throws java.io.IOException */ public void upload(InputStream input, OutputStream output, @Nullable OutputStream messages) throws IOException { - PacketLineOut pckOut = null; try { - rawIn = input; - if (messages != null) - msgOut = messages; - - if (timeout > 0) { - final Thread caller = Thread.currentThread(); - timer = new InterruptTimer(caller.getName() + "-Timer"); //$NON-NLS-1$ - TimeoutInputStream i = new TimeoutInputStream(rawIn, timer); - @SuppressWarnings("resource") - TimeoutOutputStream o = new TimeoutOutputStream(output, timer); - i.setTimeout(timeout * 1000); - o.setTimeout(timeout * 1000); - rawIn = i; - output = o; - } - - rawOut = new ResponseBufferedOutputStream(output); - if (biDirectionalPipe) { - rawOut.stopBuffering(); - } - - pckIn = new PacketLineIn(rawIn); - pckOut = new PacketLineOut(rawOut); - if (useProtocolV2()) { - serviceV2(pckOut); - } else { - service(pckOut); - } + uploadWithExceptionPropagation(input, output, messages); } catch (ServiceMayNotContinueException err) { if (!err.isOutput() && err.getMessage() != null) { try { @@ -834,6 +803,67 @@ public void upload(InputStream input, OutputStream output, throw new UploadPackInternalServerErrorException(err); } throw err; + } + } + + /** + * Execute the upload task on the socket. + * + *

+ * If the client passed extra parameters (e.g., "version=2") through a side + * channel, the caller must call setExtraParameters first to supply them. + * + * @param input + * raw input to read client commands from. Caller must ensure the + * input is buffered, otherwise read performance may suffer. + * @param output + * response back to the Git network client, to write the pack + * data onto. Caller must ensure the output is buffered, + * otherwise write performance may suffer. + * @param messages + * secondary "notice" channel to send additional messages out + * through. When run over SSH this should be tied back to the + * standard error channel of the command execution. For most + * other network connections this should be null. + * @throws ServiceMayNotContinueException + * thrown if one of the hooks throws this. + * @throws IOException + * thrown if the server or the client I/O fails, or there's an + * internal server error. + */ + public void uploadWithExceptionPropagation(InputStream input, + OutputStream output, @Nullable OutputStream messages) + throws ServiceMayNotContinueException, IOException { + try { + rawIn = input; + if (messages != null) { + msgOut = messages; + } + + if (timeout > 0) { + final Thread caller = Thread.currentThread(); + timer = new InterruptTimer(caller.getName() + "-Timer"); //$NON-NLS-1$ + TimeoutInputStream i = new TimeoutInputStream(rawIn, timer); + @SuppressWarnings("resource") + TimeoutOutputStream o = new TimeoutOutputStream(output, timer); + i.setTimeout(timeout * 1000); + o.setTimeout(timeout * 1000); + rawIn = i; + output = o; + } + + rawOut = new ResponseBufferedOutputStream(output); + if (biDirectionalPipe) { + rawOut.stopBuffering(); + } + + pckIn = new PacketLineIn(rawIn); + PacketLineOut pckOut = new PacketLineOut(rawOut); + if (useProtocolV2()) { + serviceV2(pckOut); + } else { + service(pckOut); + } } finally { msgOut = NullOutputStream.INSTANCE; walk.close();