You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by GitBox <gi...@apache.org> on 2022/11/22 04:14:57 UTC
[GitHub] [lucenenet] NightOwl888 opened a new issue, #768: Port missing test: TestIndexWriterOnJRECrash
NightOwl888 opened a new issue, #768:
URL: https://github.com/apache/lucenenet/issues/768
This test was excluded because we don't have a JRE. But just because we don't have a JRE doesn't mean we don't have to contend with runtime crashes.
The test would need to be refactored to emulate a crash on .NET. The idea is that when a crash occurs, it doesn't corrupt the index.
- [TestIndexWriterOnJRECrash.cs](https://github.com/apache/lucenenet/blob/4e49612d6867194440694b77db95fd0ed756c9a9/src/Lucene.Net.Tests/Index/TestIndexWriterOnJRECrash.cs)
- [TestIndexWriterOnJRECrash.java](https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.0/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterOnJRECrash.java)
For anyone curious what it is like to port Java code to .NET, this is a relatively small task that can satisfy your curiosity and will help to ensure Lucene.NET is as stable as it can be.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 commented on issue #768: Port missing test: TestIndexWriterOnJRECrash
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on issue #768:
URL: https://github.com/apache/lucenenet/issues/768#issuecomment-1362590769
Looks like it is coming along.
1. I think the main issue is that you are not passing the name of the test DLL to dotnet test. If not passed a name, it will run all of the tests, which could take a several minutes.
2. I don't think that `ThreadPumper` makes any sense in .NET. `ThreadPumper` is a Java-only thing. We can just attach `StandardOutput` events directly from process and it should "just work" without any "pumping". See: https://stackoverflow.com/a/285841 and https://stackoverflow.com/a/9730455
3. Since we will have a random directory name, it is important that it is passed to the spawned thread from the original thread in an environment variable so the spawned thread knows where to create the index and the final check can use the same directory after the crash to check it. You are passing one in, but the second process (in the `SetUp()` method) needs to use it if is passed in. This is critical because the test in the base class uses this value when it creates the index. We need to use `TempDir.FullName` as the value of the `"tempDir"` environment variable that is passed to the spawned process. We can also use the `"tempDir"` environment variable instead of `"tests.crashmode"` to detect which process we are running and to set the `TempDir` in `SetUp()` of the spawned process. See the example below.
4. We can eliminate everything in CrashJRE and simply throw an exception to bring down the runtime. However, we need to ensure that the crash thread `Join()` method call is before the loop for `Join()` method call. I think we will need two threads for this to work 1) A thread to run the crash (which we have) and 2) A thread to run the loop for `base.TestNRTThreads_Mem()`. The main thread should call `Join()` and wait for both of the other threads to complete. We may need some experimentation with what priority will work, though. ThreadJob will catch the exception and re-throw it when `Join()` is called, which is definitely where this needs to happen on .NET Framework. It should work on .NET Core also, but it would be a better test in that environment if we override `SafeRun(ThreadStart)` and leave the implementation empty so the exception happens during the thread run instead of at the `Join()` on the main thread.
5. For the test name matching, I think we might need to use the contains (`~`) operator instead of the equality (`=`) operator. Also the name to use [is `Name` rather than `TestName` on NUnit](https://learn.microsoft.com/en-us/dotnet/core/testing/selective-unit-tests?pivots=nunit). If that advice doesn't help, you might want to create a throwaway project with a couple of tests in it to experiment with getting it to execute one and not the other.
```c#
[SetUp]
public override void SetUp()
{
base. Setup();
var tempDir = Environment.GetEnvironmentVariable("tempDir");
if (tempDir is null)
{
TempDir = CreateTempDir("netcrash");
TempDir.Delete();
TempDir.Create();
}
else
{
TempDir = new DirectoryInfo(tempDir);
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] Jeevananthan-23 commented on issue #768: Port missing test: TestIndexWriterOnJRECrash
Posted by GitBox <gi...@apache.org>.
Jeevananthan-23 commented on issue #768:
URL: https://github.com/apache/lucenenet/issues/768#issuecomment-1375503460
@NightOwl888, I submitted the PR and look forward to your feedback. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 commented on issue #768: Port missing test: TestIndexWriterOnJRECrash
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on issue #768:
URL: https://github.com/apache/lucenenet/issues/768#issuecomment-1355228404
Hi @Jeevananthan-23. Thanks for volunteering.
I don't believe that .NET has a built-in "crash" function that can be executed from the outside of a process like Java does, so we will need to change the approach. My thought is to do something along the lines of:
1. Have the test generate the C# code to create a basic console app that runs 2 threads.
a. Thread 1 will execute the code in the `TestNRTThreads.TestNRTThreads_Mem()` method up to 10 times. This test should be modified to check for the directory to create the index in from an environment variable, and if the environment variable isn't passed to proceed with a random index directory as it currently does.
b. Thread 2 will sleep for a random number of milliseconds (between 3000 and 4000) and then throw an uncaught exception. This will crash the application on .NET Core. Inherit`ThreadJob` to make the thread, similar to how they are done in Java. `ThreadJob` automatically re-throws the exception when you call `Join`, which if we leave uncaught will also crash on .NET Framework.
2. The test would use `System.Diagnostics.Process` to call `dotnet run` to compile and run the console app. It should attach an in-memory listener to STDERR so it can confirm that an error occurred.
3. The test should create a random temp directory name and pass the name to the console app using the `EnvironmentVariables `property of `System.Diagnostics.Process.StartInfo`.
4. The test will wait for the process to exit before proceeding.
5. Once it has exited, the test should check to ensure the output of STDERR has a length greater than 0.
6. Next, proceed with the original `checkIndexes(tempDir)` test with the directory that was created in step 3.
Of course, if during your research you think you have found another approach that works, you can modify it accordingly. But please use the original `TestNRTThreads.TestNRTThreads_Mem()` and `checkIndexes(tempDir)` to generate the index and check to see that it is not corrupted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 commented on issue #768: Port missing test: TestIndexWriterOnJRECrash
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on issue #768:
URL: https://github.com/apache/lucenenet/issues/768#issuecomment-1355952643
Actually, after thinking this through a bit more, rather than wiring up a new console app, it makes more sense to simply fire up `dotnet test` and point it to the assembly that has the test in it with an [NUnit filter](https://docs.nunit.org/articles/nunit/running-tests/Test-Selection-Language.html) to only run the one test. The test framework extends NUnit and contains a lot of plumbing that will be required to get test to set up properly so this will save a lot of work.
We don't need to dynamically compile anything because the project this test is in contains the test we need to run. So by the time we run the test there is already a DLL file that we can pass to `dotnet test` to run. The test just needs to use the assembly that it is in to get the full path of the DLL file to execute. See https://stackoverflow.com/a/52956.
As for target framework, we have another test where we have done that already.
- [InstallationTest](https://github.com/apache/lucenenet/blob/c0c480409e2d3424fbdcc563426c66cacbf72632/src/dotnet/tools/Lucene.Net.Tests.Cli/InstallationTest.cs#L123-L129) reads a custom `MetaDataAttribute` where the moniker for the current target framework is compiled into the assembly.
- The build adds the `MetaDataAttribute` [here](https://github.com/apache/lucenenet/blob/c0c480409e2d3424fbdcc563426c66cacbf72632/src/dotnet/tools/Lucene.Net.Tests.Cli/Lucene.Net.Tests.Cli.csproj#L52-L58). Simply use the same approach in the `Lucene.Net.Tests._I-J.csproj` file.
The above test also has an example of using `System.Diagnostics.Process` to launch an external command.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 commented on issue #768: Port missing test: TestIndexWriterOnJRECrash
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on issue #768:
URL: https://github.com/apache/lucenenet/issues/768#issuecomment-1355954120
Of course, we still would need a second thread to kill the process, so we can use a similar approach as in Java and just subclass the original test to run and execute that test in the 2nd process. It can use the environment variable that is passing the directory of the index to determine whether it is the original test or the one that is spawned from the original.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] Jeevananthan-23 commented on issue #768: Port missing test: TestIndexWriterOnJRECrash
Posted by GitBox <gi...@apache.org>.
Jeevananthan-23 commented on issue #768:
URL: https://github.com/apache/lucenenet/issues/768#issuecomment-1356351250
Sounds Challenging to me let me dive deep into it. 🤿
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 commented on issue #768: Port missing test: TestIndexWriterOnJRECrash
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on issue #768:
URL: https://github.com/apache/lucenenet/issues/768#issuecomment-1362654504
Wait - about passing a temp directory. Let's not do that. I looked at the code and it is passing `tests.seed`, which will sync up the directory name (as well as make the spawned process repeatable).
So, let's NOT do:
```c#
[SetUp]
public override void SetUp()
{
base. Setup();
var tempDir = Environment.GetEnvironmentVariable("tempDir");
if (tempDir is null)
{
TempDir = CreateTempDir("netcrash");
TempDir.Delete();
TempDir.Create();
}
else
{
TempDir = new DirectoryInfo(tempDir);
}
}
```
And instead pass the system properties. We have renamed them in .NET using a `:` because `.` has a special meaning. This can either be passed as an environment variable or on the command line, although, the latter is a [bit of black magic](https://github.com/microsoft/vstest-docs/blob/main/docs/RunSettingsArguments.md).
You are missing a bunch of command line arguments to get this to work. This is probably the bare minimum of what we need.
```c#
ProcessStartInfo startInfo = new ProcessStartInfo
{
FileName = "dotnet",
Arguments = String.Join(" ", new[] {
"test", this.GetType().Assembly.Location,
"--framework", GetTargetFramework(),
"--filter", "Name~TestIndexWriterOnJRECrash",
"--logger\":console;verbosity=normal\"",
"--no-build",
// NOTE: This is special syntax that dotnet test supports to pass .runsettings on the command line
"--", $"RunConfiguration.TargetPlatform={GetTargetPlatform()}" }),
WorkingDirectory = theDirectory,
EnvironmentVariables = {
{ "tests:seed", RandomizedContext.CurrentContext.RandomSeedAsHex },
{ "tests:culture", Thread.CurrentThread.CurrentCulture.Name },
{ "tests:crashmode", "true" },
// passing NIGHTLY to this test makes it run for much longer, easier to catch it in the act...
{ "tests: nightly", "true" }
},
RedirectStandardOutput = true,
RedirectStandardError = true,
UseShellExecute = false
};
```
And yes, that means we can keep the existing `SetUp()` method and `tests:crashmode` environment variable code.
I am not sure how much testing has been done with passing system properties as environment variables has been done, but I know for certain the command line approach works if the above code does not.
Note that I don't know for certain this is the right filter. I got it working before, but I thought I specified `~=` or `=~` in that case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] Jeevananthan-23 commented on issue #768: Port missing test: TestIndexWriterOnJRECrash
Posted by GitBox <gi...@apache.org>.
Jeevananthan-23 commented on issue #768:
URL: https://github.com/apache/lucenenet/issues/768#issuecomment-1363030294
Let me check further it's consuming my memory a lot
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] NightOwl888 closed issue #768: Port missing test: TestIndexWriterOnJRECrash
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 closed issue #768: Port missing test: TestIndexWriterOnJRECrash
URL: https://github.com/apache/lucenenet/issues/768
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] Jeevananthan-23 commented on issue #768: Port missing test: TestIndexWriterOnJRECrash
Posted by GitBox <gi...@apache.org>.
Jeevananthan-23 commented on issue #768:
URL: https://github.com/apache/lucenenet/issues/768#issuecomment-1354767743
Hello @NightOwl888, I find this is interesting to me I could like to work on this. Totally new for FTS, Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] Jeevananthan-23 commented on issue #768: Port missing test: TestIndexWriterOnJRECrash
Posted by GitBox <gi...@apache.org>.
Jeevananthan-23 commented on issue #768:
URL: https://github.com/apache/lucenenet/issues/768#issuecomment-1362475955
I think can't test the test internally in my system maybe something wrong with my code it taking a long time to run 095802353c9a2f4b3600382050bad354583b43cf please check the code for the test if possible is there something wrong let me know. **Thanks!**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [lucenenet] rclabo commented on issue #768: Port missing test: TestIndexWriterOnJRECrash
Posted by GitBox <gi...@apache.org>.
rclabo commented on issue #768:
URL: https://github.com/apache/lucenenet/issues/768#issuecomment-1355221633
@Jeevananthan-23 Thanks for volunteering to tackle this. In general we try to port the Java code to .NET as literally as possible while also adhering to standard .NET coding conventions.
This unit test will need to be ported a bit less literally then most code since it's written to test that Lucene handles a JRE crash without corrupting the index. While we don't have a JRE in .NET, the analogous thing to test would be to test that the index does not get corrupted if the Lucene thread is terminated at a random interval.
This could be accomplished by 1) running the Lucene code on a different thread then the unit test and then terminating the Lucene thread after a random time period or 2) by having the test launch a separate process (which will of course inherently has a thread) that runs Lucene and then killing that process after a random interval.
Either approach would probably work fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org