You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by am...@apache.org on 2020/10/15 10:12:52 UTC
svn commit: r1882518 -
/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
Author: amiloslavskiy
Date: Thu Oct 15 10:12:52 2020
New Revision: 1882518
URL: http://svn.apache.org/viewvc?rev=1882518&view=rev
Log:
JavaHL: Introduce tests showing JVM crashes with TunnelAgent
The crashes will be addressed in subsequent commits.
[in subversion/bindings/javahl]
* tests/org/apache/subversion/javahl/BasicTests.java
Add helper class 'TestTunnelAgent' to emulate server replies in test
environment.
Add new tests which currently causes JVM to crash:
* testCrash_RemoteSession_nativeDispose
* testCrash_RequestChannel_nativeRead_AfterException
* testCrash_RequestChannel_nativeRead_AfterSvnError
Modified:
subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
Modified: subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
URL: http://svn.apache.org/viewvc/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java?rev=1882518&r1=1882517&r2=1882518&view=diff
==============================================================================
--- subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java (original)
+++ subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java Thu Oct 15 10:12:52 2020
@@ -25,6 +25,7 @@ package org.apache.subversion.javahl;
import static org.junit.Assert.*;
import org.apache.subversion.javahl.callback.*;
+import org.apache.subversion.javahl.remote.*;
import org.apache.subversion.javahl.types.*;
import java.io.File;
@@ -36,6 +37,7 @@ import java.io.PrintWriter;
import java.io.ByteArrayOutputStream;
import java.io.UnsupportedEncodingException;
import java.nio.ByteBuffer;
+import java.nio.channels.ClosedChannelException;
import java.nio.channels.ReadableByteChannel;
import java.nio.channels.WritableByteChannel;
import java.text.ParseException;
@@ -4417,6 +4419,298 @@ public class BasicTests extends SVNTests
assertEquals("fake", new String(revprop));
}
+ public static int FLAG_ECHO = 0x00000001;
+ public static int FLAG_THROW_IN_OPEN = 0x00000002;
+
+ public enum Actions
+ {
+ READ_CLIENT, // Read a request from SVN client
+ EMUL_SERVER, // Emulate server response
+ WAIT_TUNNEL, // Wait for tunnel to be closed
+ };
+
+ public static class ScriptItem
+ {
+ Actions action;
+ String value;
+
+ ScriptItem(Actions action, String value)
+ {
+ this.action = action;
+ this.value = value;
+ }
+ }
+
+ private static class TestTunnelAgent extends Thread
+ implements TunnelAgent
+ {
+ ScriptItem[] script;
+ int flags;
+ String error = null;
+ ReadableByteChannel request;
+ WritableByteChannel response;
+
+ final CloseTunnelCallback closeTunnelCallback = () ->
+ {
+ if ((flags & FLAG_ECHO) != 0)
+ System.out.println("TunnelAgent.CloseTunnelCallback");
+ };
+
+ TestTunnelAgent(int flags, ScriptItem[] script)
+ {
+ this.flags = flags;
+ this.script = script;
+ }
+
+ public void joinAndTest()
+ {
+ try
+ {
+ join();
+ }
+ catch (InterruptedException e)
+ {
+ fail("InterruptedException was caught");
+ }
+
+ if (error != null)
+ fail(error);
+ }
+
+ @Override
+ public boolean checkTunnel(String name)
+ {
+ return true;
+ }
+
+ private String readClient(ByteBuffer readBuffer)
+ throws IOException
+ {
+ readBuffer.reset();
+ request.read(readBuffer);
+
+ final int offset = readBuffer.arrayOffset();
+ return new String(readBuffer.array(),
+ offset,
+ readBuffer.position() - offset);
+ }
+
+ private void emulateServer(String serverMessage)
+ throws IOException
+ {
+ final byte[] responseBytes = serverMessage.getBytes();
+ response.write(ByteBuffer.wrap(responseBytes));
+ }
+
+ private void doScriptItem(ScriptItem scriptItem, ByteBuffer readBuffer)
+ throws Exception
+ {
+ switch (scriptItem.action)
+ {
+ case READ_CLIENT:
+ final String actualLine = readClient(readBuffer);
+
+ if ((flags & FLAG_ECHO) != 0)
+ {
+ System.out.println("SERVER: " + scriptItem.value);
+ System.out.flush();
+ }
+
+ if (!actualLine.contains(scriptItem.value))
+ {
+ System.err.println("Expected: " + scriptItem.value);
+ System.err.println("Actual: " + actualLine);
+ System.err.flush();
+
+ // Unblock the SVN thread by emulating a server error
+ final String serverError = "( success ( ( ) 0: ) ) ( failure ( ( 160000 39:Test script received unexpected request 0: 0 ) ) ) ";
+ emulateServer(serverError);
+
+ fail("Unexpected client request");
+ }
+ break;
+ case EMUL_SERVER:
+ if ((flags & FLAG_ECHO) != 0)
+ {
+ System.out.println("CLIENT: " + scriptItem.value);
+ System.out.flush();
+ }
+
+ emulateServer(scriptItem.value);
+ break;
+ case WAIT_TUNNEL:
+ // The loop will end with an exception when tunnel is closed
+ for (;;)
+ {
+ readClient(readBuffer);
+ }
+ }
+ }
+
+ public void run()
+ {
+ final ByteBuffer readBuffer = ByteBuffer.allocate(1024 * 1024);
+ readBuffer.mark();
+
+ for (ScriptItem scriptItem : script)
+ {
+ try {
+ doScriptItem(scriptItem, readBuffer);
+ } catch (ClosedChannelException ex) {
+ // Expected when closed properly
+ } catch (IOException e) {
+ // IOException occurs when already-freed apr_file_t was lucky
+ // to have reasonable fields to avoid the crash. It still
+ // indicates a problem.
+ error = "IOException was caught in run()";
+ return;
+ } catch (Throwable t) {
+ // No other exceptions are expected here.
+ error = "Exception was caught in run()";
+ t.printStackTrace();
+ return;
+ }
+ }
+ }
+
+ @Override
+ public CloseTunnelCallback openTunnel(ReadableByteChannel request,
+ WritableByteChannel response,
+ String name,
+ String user,
+ String hostname,
+ int port)
+ throws Throwable
+ {
+ this.request = request;
+ this.response = response;
+
+ start();
+
+ if ((flags & FLAG_THROW_IN_OPEN) != 0)
+ throw ClientException.fromException(new RuntimeException("Test exception"));
+
+ return closeTunnelCallback;
+ }
+ };
+
+ /**
+ * Test scenario which previously caused a JVM crash.
+ * In this scenario, GC is invoked before closing tunnel.
+ */
+ public void testCrash_RemoteSession_nativeDispose() {
+ final ScriptItem[] script = new ScriptItem[]{
+ new ScriptItem(Actions.EMUL_SERVER, "( success ( 2 2 ( ) ( edit-pipeline svndiff1 absent-entries commit-revprops depth log-revprops atomic-revprops partial-replay inherited-props ephemeral-txnprops file-revs-reverse ) ) ) "),
+ new ScriptItem(Actions.READ_CLIENT, "edit-pipeline"),
+ new ScriptItem(Actions.EMUL_SERVER, "( success ( ( ANONYMOUS ) 36:0113e071-0208-4a7b-9f20-3038f9caf0f0 ) ) "),
+ new ScriptItem(Actions.READ_CLIENT, "ANONYMOUS"),
+ new ScriptItem(Actions.EMUL_SERVER, "( success ( ) ) ( success ( 36:00000000-0000-0000-0000-000000000000 25:svn+test://localhost/test ( mergeinfo ) ) ) "),
+ };
+
+ final TestTunnelAgent tunnelAgent = new TestTunnelAgent(0, script);
+ final RemoteFactory remoteFactory = new RemoteFactory();
+ remoteFactory.setTunnelAgent(tunnelAgent);
+
+ ISVNRemote remote = null;
+ try {
+ remote = remoteFactory.openRemoteSession("svn+test://localhost/test", 1);
+ } catch (SubversionException e) {
+ fail("SubversionException was caught");
+ }
+
+ // 'OperationContext::openTunnel()' doesn't 'NewGlobalRef()' callback returned by 'TunnelAgent.openTunnel()'.
+ // When GC runs, it disposes the callback. When JavaHL tries to call it in 'remote.dispose()', JVM crashes.
+ System.gc();
+ remote.dispose();
+
+ tunnelAgent.joinAndTest();
+ }
+
+ /**
+ * Test scenario which previously caused a JVM crash.
+ * In this scenario, tunnel is not properly closed after exception in
+ * 'TunnelAgent.openTunnel()'.
+ */
+ public void testCrash_RequestChannel_nativeRead_AfterException()
+ {
+ // Exception causes TunnelChannel's native side to be destroyed with
+ // the following abbreviated stack:
+ // TunnelChannel.nativeClose()
+ // svn_pool_destroy(sesspool)
+ // svn_ra_open5()
+ // If TunnelAgent is unaware and calls 'RequestChannel.nativeRead()'
+ // or 'ResponseChannel.nativeWrite()', it will either crash or try to
+ // use a random file.
+ final int flags = FLAG_THROW_IN_OPEN;
+
+ final ScriptItem[] script = new ScriptItem[]{
+ new ScriptItem(Actions.EMUL_SERVER, "( success ( 2 2 ( ) ( edit-pipeline svndiff1 absent-entries commit-revprops depth log-revprops atomic-revprops partial-replay inherited-props ephemeral-txnprops file-revs-reverse ) ) ) "),
+ new ScriptItem(Actions.WAIT_TUNNEL, ""),
+ };
+
+ final TestTunnelAgent tunnelAgent = new TestTunnelAgent(flags, script);
+ final SVNClient svnClient = new SVNClient();
+ svnClient.setTunnelAgent(tunnelAgent);
+
+ try {
+ svnClient.openRemoteSession("svn+test://localhost/test");
+ } catch (SubversionException e) {
+ // RuntimeException("Test exception") is expected here
+ }
+
+ tunnelAgent.joinAndTest();
+ }
+
+ /**
+ * Test scenario which previously caused a JVM crash.
+ * In this scenario, tunnel is not properly closed after an SVN error.
+ */
+ public void testCrash_RequestChannel_nativeRead_AfterSvnError()
+ {
+ final String wcRoot = new File("tempSvnRepo").getAbsolutePath();
+
+ final ScriptItem[] script = new ScriptItem[]{
+ // openRemoteSession
+ new ScriptItem(Actions.EMUL_SERVER, "( success ( 2 2 ( ) ( edit-pipeline svndiff1 absent-entries commit-revprops depth log-revprops atomic-revprops partial-replay inherited-props ephemeral-txnprops file-revs-reverse ) ) ) "),
+ new ScriptItem(Actions.READ_CLIENT, "edit-pipeline"),
+ new ScriptItem(Actions.EMUL_SERVER, "( success ( ( ANONYMOUS ) 36:0113e071-0208-4a7b-9f20-3038f9caf0f0 ) ) "),
+ new ScriptItem(Actions.READ_CLIENT, "ANONYMOUS"),
+ new ScriptItem(Actions.EMUL_SERVER, "( success ( ) ) ( success ( 36:00000000-0000-0000-0000-000000000000 25:svn+test://localhost/test ( mergeinfo ) ) ) "),
+ // checkout
+ new ScriptItem(Actions.READ_CLIENT, "( get-latest-rev ( ) ) "),
+ // Error causes a SubversionException to be created, which then
+ // skips closing the Tunnel properly due to 'ExceptionOccurred()'
+ // in 'OperationContext::closeTunnel()'.
+ // If TunnelAgent is unaware and calls 'RequestChannel.nativeRead()',
+ // it will either crash or try to use a random file.
+ new ScriptItem(Actions.EMUL_SERVER, "( success ( ( ) 0: ) ) ( failure ( ( 160006 20:This is a test error 0: 0 ) ) ) "),
+ // TunnelAgent is not aware about the error and just keeps reading.
+ new ScriptItem(Actions.WAIT_TUNNEL, ""),
+ };
+
+ final TestTunnelAgent tunnelAgent = new TestTunnelAgent(0, script);
+ final SVNClient svnClient = new SVNClient();
+ svnClient.setTunnelAgent(tunnelAgent);
+
+ try {
+ svnClient.checkout("svn+test://localhost/test",
+ wcRoot,
+ Revision.getInstance(1),
+ null,
+ Depth.infinity,
+ true,
+ false);
+
+ svnClient.dispose();
+ } catch (ClientException ex) {
+ final int SVN_ERR_FS_NO_SUCH_REVISION = 160006;
+ if (SVN_ERR_FS_NO_SUCH_REVISION != ex.getAllMessages().get(0).getCode())
+ ex.printStackTrace();
+ }
+
+ tunnelAgent.joinAndTest();
+ }
+
/**
* @return <code>file</code> converted into a -- possibly
* <code>canonical</code>-ized -- Subversion-internal path
Re: svn commit: r1882518 -
/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Alexandr Miloslavskiy wrote on Fri, Jan 29, 2021 at 21:52:44 +0100:
> On 29.01.2021 21:47, Johan Corveleyn wrote:
> > Looks good! Looks like everything is good to go now for your javahl
> > fixes in 1.14.1 :-).
> >
> > Just to clarify: backports to bindings require only 2 votes (in fact
> > even only one +1, and one +0 or "concept +1"). Backports to core code
> > require 3 votes (or for non-LTS releases also just 2 votes). See [1]
> > for the official explanation.
> >
> > And, as you did correctly, usually the one who adds the last needed
> > vote moves the section below the "Approved changes" line (provided
> > there are no vetoes). This helps for our nightly backport bot which
> > automatically processes the STATUS file, and merges everything below
> > the Approved line (with the svn-role account).
> >
> > [1] http://subversion.apache.org/docs/community-guide/releasing.html#release-stabilization-how-many-votes
>
> Perfect, thanks for taking so much time to explain everything to me!
For future reference, automation is available for casting votes on
existing nominations and for entering new nominations; see
«tools/dist/backport.pl --help» and «tools/dist/nominate.pl --help»
(the latter is a symlink to the former). Example run:
{{{
% svn info --show-item=url
https://svn.apache.org/repos/asf/subversion/branches/1.14.x
% svn up -q -r 1886034
% svn st -q
% readlink b
../trunk/tools/dist/backport.pl
% ./b javahl
=== Candidate changes:
Skipping r1877310 (doesn't match pattern):
Add a test for issue #4711 "invalid xml file produced by svn log --xml[...]
Skipping r1883355 (doesn't match pattern):
Use the APR-1.4+ API for flushing file contents to disk.
Skipping the r1878379 group (doesn't match pattern):
Distinguish configure scripts on release mode and non release mode.
Skipping the 1.14.x-r1881534-no-crlf branch (doesn't match pattern):
r1881534 (without CRLF problem)[...]
>>> r1886029:
r1886029
Fix several crashes and JNI warnings in javahl TunnelAgent.
+1: jcorvel
Run a merge? [y,l,v,±1,±0,q,e,a, ,N,?] v
* r1886029
Fix several crashes and JNI warnings in javahl TunnelAgent.
Justification:
JavaHL shouldn't crash.
Votes:
+1: jcorvel
Run a merge? [y,l,v,±1,±0,q,e,a, ,N,?] a # In actual use you'd choose 'y' here to review/test the change, before voting on it
>>> r1886029:
r1886029
Fix several crashes and JNI warnings in javahl TunnelAgent.
+1: jcorvel
Run a merge? [y,l,v,±1,±0,q,e,a, ,N,?] +1
=== Veto-blocked changes:
Skipping the r1881534 group (doesn't match pattern):
Fix issue #4864 "build/ac-macros/macosx.m4: workaround AC_RUN_IFELSE"
=== Approved changes:
Skipping r1882234 (doesn't match pattern):
Fix file name to edit from utf8 to local style.
Skipping r1885953 (doesn't match pattern):
Fix file name encoding and quoting when invoking editor on Windows.
Skipping r1886019 (doesn't match pattern):
Fix a potential NULL dereference in the config file parser.
Skipping r1885983 (doesn't match pattern):
Fix issue #4869: 'svn info --xml' gives wrong 'source-right' of conflict
Index: STATUS
===================================================================
--- STATUS (revision 1886034)
+++ STATUS (working copy)
@@ -50,13 +50,6 @@
Votes:
+1: hartmannathan, stsp
- * r1886029
- Fix several crashes and JNI warnings in javahl TunnelAgent.
- Justification:
- JavaHL shouldn't crash.
- Votes:
- +1: jcorvel
-
Veto-blocked changes:
=====================
@@ -104,3 +97,11 @@
Bugfix; 'svn info --xml' should give correct results; user complained.
Votes:
+1: hartmannathan, stsp, jcorvel
+
+ * r1886029
+ Fix several crashes and JNI warnings in javahl TunnelAgent.
+ Justification:
+ JavaHL shouldn't crash.
+ Votes:
+ +1: jcorvel, danielsh
+
[[[
* STATUS: Vote +1 on r1886029, approving.
]]]
Commit these votes? q
}}}
I think most people just edit STATUS manually, though.
Cheers,
Daniel
Re: svn commit: r1882518 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
Posted by Nathan Hartman <ha...@gmail.com>.
On Fri, Jan 29, 2021 at 3:53 PM Alexandr Miloslavskiy
<al...@syntevo.com> wrote:
>
> On 29.01.2021 21:47, Johan Corveleyn wrote:
> > Looks good! Looks like everything is good to go now for your javahl
> > fixes in 1.14.1 :-).
> >
> > Just to clarify: backports to bindings require only 2 votes (in fact
> > even only one +1, and one +0 or "concept +1"). Backports to core code
> > require 3 votes (or for non-LTS releases also just 2 votes). See [1]
> > for the official explanation.
> >
> > And, as you did correctly, usually the one who adds the last needed
> > vote moves the section below the "Approved changes" line (provided
> > there are no vetoes). This helps for our nightly backport bot which
> > automatically processes the STATUS file, and merges everything below
> > the Approved line (with the svn-role account).
> >
> > [1] http://subversion.apache.org/docs/community-guide/releasing.html#release-stabilization-how-many-votes
>
> Perfect, thanks for taking so much time to explain everything to me!
That's great news!
We did have a few people write in to remind us not to forget to
remember to include it in 1.14.1... They'll be happy to hear that
it'll be there.
Cheers,
Nathan
Re: svn commit: r1882518 -
/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
Posted by Alexandr Miloslavskiy <al...@syntevo.com>.
On 29.01.2021 21:47, Johan Corveleyn wrote:
> Looks good! Looks like everything is good to go now for your javahl
> fixes in 1.14.1 :-).
>
> Just to clarify: backports to bindings require only 2 votes (in fact
> even only one +1, and one +0 or "concept +1"). Backports to core code
> require 3 votes (or for non-LTS releases also just 2 votes). See [1]
> for the official explanation.
>
> And, as you did correctly, usually the one who adds the last needed
> vote moves the section below the "Approved changes" line (provided
> there are no vetoes). This helps for our nightly backport bot which
> automatically processes the STATUS file, and merges everything below
> the Approved line (with the svn-role account).
>
> [1] http://subversion.apache.org/docs/community-guide/releasing.html#release-stabilization-how-many-votes
Perfect, thanks for taking so much time to explain everything to me!
Re: svn commit: r1882518 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
Posted by Johan Corveleyn <jc...@gmail.com>.
On Fri, Jan 29, 2021 at 9:29 PM Alexandr Miloslavskiy
<al...@syntevo.com> wrote:
>
> On 29.01.2021 20:51, Johan Corveleyn wrote:
>
> > Fingers crossed for a second vote in STATUS now :-).
>
> Did that, hopefully I didn't mess anything up, it's my first experience
> with it :)
Looks good! Looks like everything is good to go now for your javahl
fixes in 1.14.1 :-).
Just to clarify: backports to bindings require only 2 votes (in fact
even only one +1, and one +0 or "concept +1"). Backports to core code
require 3 votes (or for non-LTS releases also just 2 votes). See [1]
for the official explanation.
And, as you did correctly, usually the one who adds the last needed
vote moves the section below the "Approved changes" line (provided
there are no vetoes). This helps for our nightly backport bot which
automatically processes the STATUS file, and merges everything below
the Approved line (with the svn-role account).
[1] http://subversion.apache.org/docs/community-guide/releasing.html#release-stabilization-how-many-votes
--
Johan
Re: svn commit: r1882518 -
/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
Posted by Alexandr Miloslavskiy <al...@syntevo.com>.
On 29.01.2021 20:51, Johan Corveleyn wrote:
> Fingers crossed for a second vote in STATUS now :-).
Did that, hopefully I didn't mess anything up, it's my first experience
with it :)
Re: svn commit: r1882518 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
Posted by Johan Corveleyn <jc...@gmail.com>.
On Thu, Jan 28, 2021 at 11:28 AM Johan Corveleyn <jc...@gmail.com> wrote:
...
> With that, the branch looks good to me, and I think we should merge
> this to trunk, and then nominate it (the merge-to-trunk commit I
> guess) for backport to 1.14 (concretely, this means adding an entry to
> 1.14.x/STATUS [1]).
>
> @Alexandr: would you like to do the honors? First to merge your branch
> to trunk, and after that to add a corresponding section to
> 1.14.x/STATUS?
>
> In fact: please consider your partial committership (javahl bindings)
> now to be no longer confined to branches. Feel free to perform commits
> to the javahl bindings area also on trunk (which includes the above
> merge of course).
> This also means you have a binding vote on backport proposals related
> to the javahl bindings.
>
> [1] https://svn.apache.org/repos/asf/subversion/branches/1.14.x/STATUS
Small update for whoever is following along: I just merged the branch
to trunk in r1886029, and nominated it in 1.14.x/STATUS.
To assure the list that I'm not bypassing Alexandr here: we mailed
offlist about this, and he preferred that I would perform the merge,
so I did :-).
Fingers crossed for a second vote in STATUS now :-).
--
Johan
Re: svn commit: r1882518 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
Posted by Johan Corveleyn <jc...@gmail.com>.
On Thu, Jan 28, 2021 at 1:15 AM Alexandr Miloslavskiy
<al...@syntevo.com> wrote:
>
> On 27.01.2021 13:00, Johan Corveleyn wrote:
>
> > ^^ some lines indented with tabs instead of spaces
>
> > ^^ curly brace should be on next line
>
> > ^^ The comment above, describing how the JVM crashes, refers to the
> > situation before you actually fixed that :-). Can you rephrase it a
> > bit, so it is still applicable even with the crash now fixed?
>
> > ^^ same here, the comments describe the behavior before the fix, maybe
> > rephrase it so it's still valid after you fixed it.
>
> Thanks, I fixed everything in r1885955.
Thanks. I see you did a bit more than what I mentioned above (I guess
you did a "reformat" of the entire section you added), but that's fine
of course (I only spotted a couple of tidbits, not everything :)).
With that, the branch looks good to me, and I think we should merge
this to trunk, and then nominate it (the merge-to-trunk commit I
guess) for backport to 1.14 (concretely, this means adding an entry to
1.14.x/STATUS [1]).
@Alexandr: would you like to do the honors? First to merge your branch
to trunk, and after that to add a corresponding section to
1.14.x/STATUS?
In fact: please consider your partial committership (javahl bindings)
now to be no longer confined to branches. Feel free to perform commits
to the javahl bindings area also on trunk (which includes the above
merge of course).
This also means you have a binding vote on backport proposals related
to the javahl bindings.
[1] https://svn.apache.org/repos/asf/subversion/branches/1.14.x/STATUS
--
Johan
Re: svn commit: r1882518 -
/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
Posted by Alexandr Miloslavskiy <al...@syntevo.com>.
On 27.01.2021 13:00, Johan Corveleyn wrote:
> ^^ some lines indented with tabs instead of spaces
> ^^ curly brace should be on next line
> ^^ The comment above, describing how the JVM crashes, refers to the
> situation before you actually fixed that :-). Can you rephrase it a
> bit, so it is still applicable even with the crash now fixed?
> ^^ same here, the comments describe the behavior before the fix, maybe
> rephrase it so it's still valid after you fixed it.
Thanks, I fixed everything in r1885955.
Re: svn commit: r1882518 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
Posted by Johan Corveleyn <jc...@gmail.com>.
On Thu, Oct 15, 2020 at 12:12 PM <am...@apache.org> wrote:
>
> Author: amiloslavskiy
> Date: Thu Oct 15 10:12:52 2020
> New Revision: 1882518
>
> URL: http://svn.apache.org/viewvc?rev=1882518&view=rev
> Log:
> JavaHL: Introduce tests showing JVM crashes with TunnelAgent
>
> The crashes will be addressed in subsequent commits.
>
> [in subversion/bindings/javahl]
> * tests/org/apache/subversion/javahl/BasicTests.java
> Add helper class 'TestTunnelAgent' to emulate server replies in test
> environment.
> Add new tests which currently causes JVM to crash:
> * testCrash_RemoteSession_nativeDispose
> * testCrash_RequestChannel_nativeRead_AfterException
> * testCrash_RequestChannel_nativeRead_AfterSvnError
A couple more small remarks here below ...
<snip>
> + private String readClient(ByteBuffer readBuffer)
> + throws IOException
> + {
> + readBuffer.reset();
> + request.read(readBuffer);
> +
> + final int offset = readBuffer.arrayOffset();
> + return new String(readBuffer.array(),
> + offset,
> + readBuffer.position() - offset);
> + }
> +
> + private void emulateServer(String serverMessage)
> + throws IOException
> + {
> + final byte[] responseBytes = serverMessage.getBytes();
> + response.write(ByteBuffer.wrap(responseBytes));
> + }
^^ some lines indented with tabs instead of spaces
<snip>
> + public void testCrash_RemoteSession_nativeDispose() {
^^ curly brace should be on next line
<snip>
> + // 'OperationContext::openTunnel()' doesn't 'NewGlobalRef()' callback returned by 'TunnelAgent.openTunnel()'.
> + // When GC runs, it disposes the callback. When JavaHL tries to call it in 'remote.dispose()', JVM crashes.
^^ The comment above, describing how the JVM crashes, refers to the
situation before you actually fixed that :-). Can you rephrase it a
bit, so it is still applicable even with the crash now fixed?
<snip>
> + public void testCrash_RequestChannel_nativeRead_AfterSvnError()
> + {
> + final String wcRoot = new File("tempSvnRepo").getAbsolutePath();
> +
> + final ScriptItem[] script = new ScriptItem[]{
> + // openRemoteSession
> + new ScriptItem(Actions.EMUL_SERVER, "( success ( 2 2 ( ) ( edit-pipeline svndiff1 absent-entries commit-revprops depth log-revprops atomic-revprops partial-replay inherited-props ephemeral-txnprops file-revs-reverse ) ) ) "),
> + new ScriptItem(Actions.READ_CLIENT, "edit-pipeline"),
> + new ScriptItem(Actions.EMUL_SERVER, "( success ( ( ANONYMOUS ) 36:0113e071-0208-4a7b-9f20-3038f9caf0f0 ) ) "),
> + new ScriptItem(Actions.READ_CLIENT, "ANONYMOUS"),
> + new ScriptItem(Actions.EMUL_SERVER, "( success ( ) ) ( success ( 36:00000000-0000-0000-0000-000000000000 25:svn+test://localhost/test ( mergeinfo ) ) ) "),
> + // checkout
> + new ScriptItem(Actions.READ_CLIENT, "( get-latest-rev ( ) ) "),
> + // Error causes a SubversionException to be created, which then
> + // skips closing the Tunnel properly due to 'ExceptionOccurred()'
> + // in 'OperationContext::closeTunnel()'.
> + // If TunnelAgent is unaware and calls 'RequestChannel.nativeRead()',
> + // it will either crash or try to use a random file.
> + new ScriptItem(Actions.EMUL_SERVER, "( success ( ( ) 0: ) ) ( failure ( ( 160006 20:This is a test error 0: 0 ) ) ) "),
> + // TunnelAgent is not aware about the error and just keeps reading.
^^ same here, the comments describe the behavior before the fix, maybe
rephrase it so it's still valid after you fixed it.
--
Johan