You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@eventmesh.apache.org by "tylerbertrand (via GitHub)" <gi...@apache.org> on 2024/01/03 19:00:23 UTC
[PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
tylerbertrand opened a new pull request, #4716:
URL: https://github.com/apache/eventmesh/pull/4716
<!--
### Contribution Checklist
- Name the pull request in the form "[ISSUE #XXXX] Title of the pull request",
where *XXXX* should be replaced by the actual issue number.
Skip *[ISSUE #XXXX]* if there is no associated github issue for this pull request.
- Fill out the template below to describe the changes contributed by the pull request.
That will give reviewers the context they need to do the review.
- Each pull request should address only one issue.
Please do not mix up code from multiple issues.
- Each commit in the pull request should have a meaningful commit message.
- Once all items of the checklist are addressed, remove the above text and this checklist,
leaving only the filled out template below.
(The sections below can be removed for hotfixes of typos)
-->
<!--
(If this PR fixes a GitHub issue, please add `Fixes #<XXX>` or `Closes #<XXX>`.)
-->
Fixes https://github.com/apache/eventmesh/issues/4711
### Motivation
The `@TempDir` added in the [Build caching optimizations PR](https://github.com/apache/eventmesh/pull/4689) caused tests to fail when run on a Windows machine. The failure was due to the fact that the directory could not be deleted because it was in use by other processes.
### Modifications
The resources holding on to the temporary config file that were preventing the temp directory from being deleted are now properly closed.
### Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented?
- If a feature is not applicable for documentation, explain why?
- If a feature is not documented yet in this PR, please create a followup issue for adding the documentation
--
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@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: dev-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on code in PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#discussion_r1446252750
##########
eventmesh-common/src/main/java/org/apache/eventmesh/common/file/WatchFileTask.java:
##########
@@ -55,11 +56,21 @@ public WatchFileTask(String directoryPath) {
throw new IllegalArgumentException("must be a file directory : " + directoryPath);
}
- try (WatchService watchService = FILE_SYSTEM.newWatchService()) {
- this.watchService = watchService;
+ try {
+ this.watchService = FILE_SYSTEM.newWatchService();
+ } catch (IOException ex) {
+ throw new RuntimeException("WatchService initialization fail", ex);
+ }
+
+ try {
path.register(this.watchService, StandardWatchEventKinds.OVERFLOW, StandardWatchEventKinds.ENTRY_MODIFY,
StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE);
- } catch (Exception ex) {
+ } catch (IOException ex) {
+ try {
+ this.watchService.close();
Review Comment:
@tylerbertrand What @pandaapo mentioned should be here.
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "tylerbertrand (via GitHub)" <gi...@apache.org>.
tylerbertrand commented on code in PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#discussion_r1444870175
##########
eventmesh-common/src/test/java/org/apache/eventmesh/common/file/WatchFileManagerTest.java:
##########
@@ -40,31 +38,32 @@ public class WatchFileManagerTest {
@Test
public void testWatchFile() throws IOException, InterruptedException {
String file = WatchFileManagerTest.class.getResource("/configuration.properties").getFile();
- File f = new File(file);
+ File configFile = new File(file);
File tempConfigFile = new File(tempConfigDir, "configuration.properties");
- Files.copy(f.toPath(), tempConfigFile.toPath());
-
- final FileChangeListener fileChangeListener = new FileChangeListener() {
+ Files.copy(configFile.toPath(), tempConfigFile.toPath());
- @Override
- public void onChanged(FileChangeContext changeContext) {
- Assertions.assertEquals(tempConfigFile.getName(), changeContext.getFileName());
- Assertions.assertEquals(tempConfigFile.getParent(), changeContext.getDirectoryPath());
- }
+ final FileChangeListener mockFileChangeListener = Mockito.mock(FileChangeListener.class);
+ Mockito.when(mockFileChangeListener.support(
+ Mockito.argThat(isFileUnderTest(tempConfigFile.getParent(), tempConfigFile.getName())))
+ ).thenReturn(true);
- @Override
- public boolean support(FileChangeContext changeContext) {
- return changeContext.getWatchEvent().context().toString().contains(tempConfigFile.getName());
- }
- };
- WatchFileManager.registerFileChangeListener(tempConfigFile.getParent(), fileChangeListener);
+ WatchFileManager.registerFileChangeListener(tempConfigFile.getParent(), mockFileChangeListener);
Properties properties = new Properties();
- properties.load(new BufferedReader(new FileReader(tempConfigFile)));
- properties.setProperty("eventMesh.server.newAdd", "newAdd");
- FileWriter fw = new FileWriter(tempConfigFile);
- properties.store(fw, "newAdd");
+ try (
+ BufferedReader bufferedReader = new BufferedReader(new FileReader(configFile));
Review Comment:
Because creating a new `FileWriter` actually clears the file, the temp config file was empty when `Properties.load()` was called, so I instead created the reader on the original config file to ensure that all properties were read. I'll just split this into two try-with-resources blocks instead - I think that is a better way to handle it. The first block will create the buffered reader and load the properties from the temp config file, the second will create the file writer and write the updated properties.
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "tylerbertrand (via GitHub)" <gi...@apache.org>.
tylerbertrand commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1879219077
Ok, I realized the problem is that the assertions were actually being executed, but they were failing in another thread, so they did not fail the test.
To get around this, I pushed a commit that restructures the test to use Mockito to verify that the `onChanged()` method was called for the file under test. This correctly tests whether the change to the file was detected or not.
The one quirk is that it seems to take a bit for the `WatchService.take()` method to pick up the change to the file - it was about 10s on my machine. I added a 15s timeout to the `Mockito.verify()` call to account for this. With the timeout configured, the test will pass as soon as the `onChange()` method is called - it doesn't need to wait the full 15s if it is called sooner.
Let me know what you think.
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1879695776
The https://github.com/apache/eventmesh/pull/4716/commits/7bea5512676dd9972974fcc4814bed2088b41925 passed the test however the https://github.com/apache/eventmesh/pull/4716/commits/277273110656dc4813e97de8773c4756386470d7 failed on my IDEA (win11).
```java
org.mockito.exceptions.verification.TooManyActualInvocations:
fileChangeListener.onChanged(
<custom argument matcher>
);
Wanted 1 time:
-> at org.apache.eventmesh.common.file.WatchFileManagerTest.testWatchFile(WatchFileManagerTest.java:63)
But was 2 times:
-> at org.apache.eventmesh.common.file.WatchFileTask.precessWatchEvent(WatchFileTask.java:127)
-> at org.apache.eventmesh.common.file.WatchFileTask.precessWatchEvent(WatchFileTask.java:127)
at org.apache.eventmesh.common.file.WatchFileManagerTest.testWatchFile(WatchFileManagerTest.java:63)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:727)
at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:156)
at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:147)
at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:86)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(InterceptingExecutableInvoker.java:103)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:92)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:86)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$7(TestMethodTestDescriptor.java:217)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:213)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:138)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:68)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:151)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
at java.util.ArrayList.forEach(ArrayList.java:1259)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
at java.util.ArrayList.forEach(ArrayList.java:1259)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:147)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:127)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:90)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:55)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:102)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:54)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:86)
at org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:86)
at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.processAllTestClasses(JUnitPlatformTestClassProcessor.java:110)
at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.access$000(JUnitPlatformTestClassProcessor.java:90)
at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor.stop(JUnitPlatformTestClassProcessor.java:85)
at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:62)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
at com.sun.proxy.$Proxy2.stop(Unknown Source)
at org.gradle.api.internal.tasks.testing.worker.TestWorker$3.run(TestWorker.java:193)
at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:113)
at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:65)
at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)
```
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "tylerbertrand (via GitHub)" <gi...@apache.org>.
tylerbertrand commented on code in PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#discussion_r1445019540
##########
eventmesh-common/src/test/java/org/apache/eventmesh/common/file/WatchFileManagerTest.java:
##########
@@ -40,31 +38,32 @@ public class WatchFileManagerTest {
@Test
public void testWatchFile() throws IOException, InterruptedException {
String file = WatchFileManagerTest.class.getResource("/configuration.properties").getFile();
- File f = new File(file);
+ File configFile = new File(file);
File tempConfigFile = new File(tempConfigDir, "configuration.properties");
- Files.copy(f.toPath(), tempConfigFile.toPath());
-
- final FileChangeListener fileChangeListener = new FileChangeListener() {
+ Files.copy(configFile.toPath(), tempConfigFile.toPath());
- @Override
- public void onChanged(FileChangeContext changeContext) {
- Assertions.assertEquals(tempConfigFile.getName(), changeContext.getFileName());
- Assertions.assertEquals(tempConfigFile.getParent(), changeContext.getDirectoryPath());
- }
+ final FileChangeListener mockFileChangeListener = Mockito.mock(FileChangeListener.class);
+ Mockito.when(mockFileChangeListener.support(
+ Mockito.argThat(isFileUnderTest(tempConfigFile.getParent(), tempConfigFile.getName())))
+ ).thenReturn(true);
- @Override
- public boolean support(FileChangeContext changeContext) {
- return changeContext.getWatchEvent().context().toString().contains(tempConfigFile.getName());
- }
- };
- WatchFileManager.registerFileChangeListener(tempConfigFile.getParent(), fileChangeListener);
+ WatchFileManager.registerFileChangeListener(tempConfigFile.getParent(), mockFileChangeListener);
Properties properties = new Properties();
- properties.load(new BufferedReader(new FileReader(tempConfigFile)));
- properties.setProperty("eventMesh.server.newAdd", "newAdd");
- FileWriter fw = new FileWriter(tempConfigFile);
- properties.store(fw, "newAdd");
+ try (
+ BufferedReader bufferedReader = new BufferedReader(new FileReader(configFile));
+ FileWriter fw = new FileWriter(tempConfigFile)
+ ) {
+ properties.load(bufferedReader);
+ properties.setProperty("eventMesh.server.newAdd", "newAdd");
+ properties.store(fw, "newAdd");
+ }
+
+ Mockito.verify(mockFileChangeListener, Mockito.timeout(15_000))
Review Comment:
@Pil0tXia see above for the likely reason why the test failed
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on code in PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#discussion_r1445851932
##########
eventmesh-common/src/main/java/org/apache/eventmesh/common/file/WatchFileTask.java:
##########
@@ -55,11 +56,21 @@ public WatchFileTask(String directoryPath) {
throw new IllegalArgumentException("must be a file directory : " + directoryPath);
}
- try (WatchService watchService = FILE_SYSTEM.newWatchService()) {
- this.watchService = watchService;
+ try {
+ this.watchService = FILE_SYSTEM.newWatchService();
+ } catch (IOException ex) {
+ throw new RuntimeException("WatchService initialization fail", ex);
+ }
+
+ try {
path.register(this.watchService, StandardWatchEventKinds.OVERFLOW, StandardWatchEventKinds.ENTRY_MODIFY,
StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE);
- } catch (Exception ex) {
+ } catch (IOException ex) {
+ try {
+ this.watchService.close();
Review Comment:
The `watchService` created in `WatchFileTask` will not close until the EventMesh server shutdown now. The `WatchFileTask` instances are few and I think it's 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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1876188695
## [Codecov](https://app.codecov.io/gh/apache/eventmesh/pull/4716?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`e507497`)](https://app.codecov.io/gh/apache/eventmesh/commit/e507497119277fff68790d0eeb63bce0530cee26?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 17.40% compared to head [(`b852ef4`)](https://app.codecov.io/gh/apache/eventmesh/pull/4716?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 17.38%.
> Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #4716 +/- ##
============================================
- Coverage 17.40% 17.38% -0.02%
Complexity 1759 1759
============================================
Files 797 797
Lines 29856 29845 -11
Branches 2579 2563 -16
============================================
- Hits 5195 5189 -6
+ Misses 24180 24176 -4
+ Partials 481 480 -1
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/eventmesh/pull/4716?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "tylerbertrand (via GitHub)" <gi...@apache.org>.
tylerbertrand commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1877261171
@Pil0tXia I'm not sure what is causing these exceptions, but it shouldn't have anything to do with caching. Caching would only affect whether or not the tests are executed, not their behavior.
I executed the tests on a commit before we enabled caching and before we made any changes to this test, and the exceptions are still being logged there.
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "tylerbertrand (via GitHub)" <gi...@apache.org>.
tylerbertrand commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1877575944
@Pil0tXia it's happening on my Mac as well
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1876291348
Am I encountering these exceptions because the cache hasn't been updated?
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "tylerbertrand (via GitHub)" <gi...@apache.org>.
tylerbertrand commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1877597101
@Pil0tXia Is that something you wanted to address in this PR or is this one good to go?
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "tylerbertrand (via GitHub)" <gi...@apache.org>.
tylerbertrand commented on code in PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#discussion_r1446304248
##########
eventmesh-common/src/main/java/org/apache/eventmesh/common/file/WatchFileTask.java:
##########
@@ -55,11 +56,21 @@ public WatchFileTask(String directoryPath) {
throw new IllegalArgumentException("must be a file directory : " + directoryPath);
}
- try (WatchService watchService = FILE_SYSTEM.newWatchService()) {
- this.watchService = watchService;
+ try {
+ this.watchService = FILE_SYSTEM.newWatchService();
+ } catch (IOException ex) {
+ throw new RuntimeException("WatchService initialization fail", ex);
+ }
+
+ try {
path.register(this.watchService, StandardWatchEventKinds.OVERFLOW, StandardWatchEventKinds.ENTRY_MODIFY,
StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE);
- } catch (Exception ex) {
+ } catch (IOException ex) {
+ try {
+ this.watchService.close();
Review Comment:
@Pil0tXia @pandaapo Is [this commit](https://github.com/apache/eventmesh/pull/4716/commits/989c9629065e0f2f1678d48ca7ab29d326632bf1) what you had in mind?
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on code in PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#discussion_r1446345311
##########
eventmesh-common/src/main/java/org/apache/eventmesh/common/file/WatchFileTask.java:
##########
@@ -55,11 +56,21 @@ public WatchFileTask(String directoryPath) {
throw new IllegalArgumentException("must be a file directory : " + directoryPath);
}
- try (WatchService watchService = FILE_SYSTEM.newWatchService()) {
- this.watchService = watchService;
+ try {
+ this.watchService = FILE_SYSTEM.newWatchService();
+ } catch (IOException ex) {
+ throw new RuntimeException("WatchService initialization fail", ex);
+ }
+
+ try {
path.register(this.watchService, StandardWatchEventKinds.OVERFLOW, StandardWatchEventKinds.ENTRY_MODIFY,
StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE);
- } catch (Exception ex) {
+ } catch (IOException ex) {
+ try {
+ this.watchService.close();
Review Comment:
Please see my latest https://github.com/apache/eventmesh/pull/4716#pullrequestreview-1811550992
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on code in PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#discussion_r1443988771
##########
eventmesh-common/src/test/java/org/apache/eventmesh/common/file/WatchFileManagerTest.java:
##########
@@ -40,31 +38,32 @@ public class WatchFileManagerTest {
@Test
public void testWatchFile() throws IOException, InterruptedException {
String file = WatchFileManagerTest.class.getResource("/configuration.properties").getFile();
- File f = new File(file);
+ File configFile = new File(file);
File tempConfigFile = new File(tempConfigDir, "configuration.properties");
- Files.copy(f.toPath(), tempConfigFile.toPath());
-
- final FileChangeListener fileChangeListener = new FileChangeListener() {
+ Files.copy(configFile.toPath(), tempConfigFile.toPath());
- @Override
- public void onChanged(FileChangeContext changeContext) {
- Assertions.assertEquals(tempConfigFile.getName(), changeContext.getFileName());
- Assertions.assertEquals(tempConfigFile.getParent(), changeContext.getDirectoryPath());
- }
+ final FileChangeListener mockFileChangeListener = Mockito.mock(FileChangeListener.class);
+ Mockito.when(mockFileChangeListener.support(
+ Mockito.argThat(isFileUnderTest(tempConfigFile.getParent(), tempConfigFile.getName())))
+ ).thenReturn(true);
- @Override
- public boolean support(FileChangeContext changeContext) {
- return changeContext.getWatchEvent().context().toString().contains(tempConfigFile.getName());
- }
- };
- WatchFileManager.registerFileChangeListener(tempConfigFile.getParent(), fileChangeListener);
+ WatchFileManager.registerFileChangeListener(tempConfigFile.getParent(), mockFileChangeListener);
Properties properties = new Properties();
- properties.load(new BufferedReader(new FileReader(tempConfigFile)));
- properties.setProperty("eventMesh.server.newAdd", "newAdd");
- FileWriter fw = new FileWriter(tempConfigFile);
- properties.store(fw, "newAdd");
+ try (
+ BufferedReader bufferedReader = new BufferedReader(new FileReader(configFile));
+ FileWriter fw = new FileWriter(tempConfigFile)
+ ) {
+ properties.load(bufferedReader);
+ properties.setProperty("eventMesh.server.newAdd", "newAdd");
+ properties.store(fw, "newAdd");
+ }
+
+ Mockito.verify(mockFileChangeListener, Mockito.timeout(15_000))
Review Comment:
Perhaps you can use `Mockito.timeout(15_000).atLeast(1)` to address `TooManyActualInvocations` error in certain environments. But I don't know why the error occurs in some environments and not in others.
##########
eventmesh-common/src/test/java/org/apache/eventmesh/common/file/WatchFileManagerTest.java:
##########
@@ -40,31 +38,32 @@ public class WatchFileManagerTest {
@Test
public void testWatchFile() throws IOException, InterruptedException {
String file = WatchFileManagerTest.class.getResource("/configuration.properties").getFile();
- File f = new File(file);
+ File configFile = new File(file);
File tempConfigFile = new File(tempConfigDir, "configuration.properties");
- Files.copy(f.toPath(), tempConfigFile.toPath());
-
- final FileChangeListener fileChangeListener = new FileChangeListener() {
+ Files.copy(configFile.toPath(), tempConfigFile.toPath());
- @Override
- public void onChanged(FileChangeContext changeContext) {
- Assertions.assertEquals(tempConfigFile.getName(), changeContext.getFileName());
- Assertions.assertEquals(tempConfigFile.getParent(), changeContext.getDirectoryPath());
- }
+ final FileChangeListener mockFileChangeListener = Mockito.mock(FileChangeListener.class);
+ Mockito.when(mockFileChangeListener.support(
+ Mockito.argThat(isFileUnderTest(tempConfigFile.getParent(), tempConfigFile.getName())))
+ ).thenReturn(true);
- @Override
- public boolean support(FileChangeContext changeContext) {
- return changeContext.getWatchEvent().context().toString().contains(tempConfigFile.getName());
- }
- };
- WatchFileManager.registerFileChangeListener(tempConfigFile.getParent(), fileChangeListener);
+ WatchFileManager.registerFileChangeListener(tempConfigFile.getParent(), mockFileChangeListener);
Properties properties = new Properties();
- properties.load(new BufferedReader(new FileReader(tempConfigFile)));
- properties.setProperty("eventMesh.server.newAdd", "newAdd");
- FileWriter fw = new FileWriter(tempConfigFile);
- properties.store(fw, "newAdd");
+ try (
+ BufferedReader bufferedReader = new BufferedReader(new FileReader(configFile));
Review Comment:
Should `tempConfigFile` be used to create `FileReader` here?
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1877404048
> I executed the tests on a commit before we enabled caching and before we made any changes to this test, and the exceptions are still being logged there.
Same on my end. This exception has gone unnoticed by both the CI and non-Windows platform developers. I think we need to invoke the `org.apache.eventmesh.common.file.WatchFileTask#shutdown` method to stop the execution of `watchService.take()`.
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "tylerbertrand (via GitHub)" <gi...@apache.org>.
tylerbertrand commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1883508175
@Pil0tXia Ah, my misunderstanding, thanks for clarifying. I re-added closing `watchService` in the exception handling.
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "tylerbertrand (via GitHub)" <gi...@apache.org>.
tylerbertrand commented on code in PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#discussion_r1446213729
##########
eventmesh-common/src/main/java/org/apache/eventmesh/common/file/WatchFileTask.java:
##########
@@ -55,11 +56,21 @@ public WatchFileTask(String directoryPath) {
throw new IllegalArgumentException("must be a file directory : " + directoryPath);
}
- try (WatchService watchService = FILE_SYSTEM.newWatchService()) {
- this.watchService = watchService;
+ try {
+ this.watchService = FILE_SYSTEM.newWatchService();
+ } catch (IOException ex) {
+ throw new RuntimeException("WatchService initialization fail", ex);
+ }
+
+ try {
path.register(this.watchService, StandardWatchEventKinds.OVERFLOW, StandardWatchEventKinds.ENTRY_MODIFY,
StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE);
- } catch (Exception ex) {
+ } catch (IOException ex) {
+ try {
+ this.watchService.close();
Review Comment:
Are you saying this does not to be closed here?
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo merged PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716
--
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@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: dev-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1878082099
@tylerbertrand I would appreciate your assistance in resolving this thrown exception.
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1877365974
I have two questions.
1. Why does this unit test run successfully in CI?
2. Is this bug caused by erroneously closing `this.watchService` object in `WatchFileTask` constructor called in `WatchFileManager.registerFileChangeListener()`?
```
private final transient WatchService watchService;
...
public WatchFileTask(String directoryPath) {
...
try (WatchService watchService = FILE_SYSTEM.newWatchService()) {
this.watchService = watchService;
path.register(this.watchService, StandardWatchEventKinds.OVERFLOW, StandardWatchEventKinds.ENTRY_MODIFY,
StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE);
} catch (Exception ex) {
throw new UnsupportedOperationException("WatchService registry fail", ex);
}
}
```
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "tylerbertrand (via GitHub)" <gi...@apache.org>.
tylerbertrand commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1878852877
@Pil0tXia Sure, I'll see what I can do.
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on code in PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#discussion_r1446331535
##########
eventmesh-common/src/main/java/org/apache/eventmesh/common/file/WatchFileTask.java:
##########
@@ -83,6 +78,11 @@ public void addFileChangeListener(FileChangeListener fileChangeListener) {
public void shutdown() {
watch = false;
+ try {
+ this.watchService.close();
+ } catch (IOException e) {
+ throw new RuntimeException("Unable to close WatchService", e);
+ }
Review Comment:
In your latest commit, you added the shutdown handling for the `watchService` in the `WatchFileTask#shutdown()` method. However, this doesn't actually change anything because the `shutdown()` method is only called before the application is closed. Nevertheless, I think the added code can still be kept.
##########
eventmesh-common/src/main/java/org/apache/eventmesh/common/file/WatchFileTask.java:
##########
@@ -66,11 +66,6 @@ public WatchFileTask(String directoryPath) {
path.register(this.watchService, StandardWatchEventKinds.OVERFLOW, StandardWatchEventKinds.ENTRY_MODIFY,
StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE);
} catch (IOException ex) {
- try {
- this.watchService.close();
- } catch (IOException e) {
- ex.addSuppressed(e);
- }
Review Comment:
However, you removed the part where the `watchService` is closed in the exception handling, and I think that this part should be retained.
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "tylerbertrand (via GitHub)" <gi...@apache.org>.
tylerbertrand commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1879016086
@Pil0tXia @pandaapo So the exception is caused by the fact that the `WatchService` is created using try-with-resources, which means the `WatchService` is closed prematurely as soon as the try-with-resources block is exited.
I don't have so much domain knowledge here, but would [this latest commit](https://github.com/apache/eventmesh/pull/4716/commits/7bea5512676dd9972974fcc4814bed2088b41925) be a reasonable fix? If the watch service fails to initialize, there is nothing to close, so a `RuntimeException` is thrown. If it is created successfully, and it is the registration that fails, then when handling the exception, we can close the `WatchService`.
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1879561146
> @Pil0tXia @pandaapo So the exception is caused by the fact that the `WatchService` is created using try-with-resources, which means the `WatchService` is closed prematurely as soon as the try-with-resources block is exited.
>
> I don't have so much domain knowledge here, but would [this latest commit](https://github.com/apache/eventmesh/pull/4716/commits/7bea5512676dd9972974fcc4814bed2088b41925) be a reasonable fix? If the watch service fails to initialize, there is nothing to close, so a `RuntimeException` is thrown. If it is created successfully, and it is the registration that fails, then when handling the exception, we can close the `WatchService`.
The meaning I expressed in point 2 of https://github.com/apache/eventmesh/pull/4716#issuecomment-1877365974 is consistent with the modification you made here.
> To get around this, I pushed a commit that restructures the test to use Mockito to verify that the `onChanged()` method was called for the file under test. This correctly tests whether the change to the file was detected or not.
>
> The one quirk is that it seems to take a bit for the `WatchService.take()` method to pick up the change to the file - it was about 10s on my machine. I added a 15s timeout to the `Mockito.verify()` call to account for this. With the timeout configured, the test will pass as soon as the `onChange()` method is called - it doesn't need to wait the full 15s if it is called sooner. There is the potential for flakiness here, but I'm not sure how else to account for that without some other, more in depth restructuring.
>
> Let me know what you think.
I agree with the validation approach you modified.
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "tylerbertrand (via GitHub)" <gi...@apache.org>.
tylerbertrand commented on PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#issuecomment-1879044273
Hmm...it looks like the assertions in the test are still not being called
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org
Re: [PR] [ISSUE #4711] Properly close resources used by WatchFileManagerTest (eventmesh)
Posted by "tylerbertrand (via GitHub)" <gi...@apache.org>.
tylerbertrand commented on code in PR #4716:
URL: https://github.com/apache/eventmesh/pull/4716#discussion_r1444860760
##########
eventmesh-common/src/test/java/org/apache/eventmesh/common/file/WatchFileManagerTest.java:
##########
@@ -40,31 +38,32 @@ public class WatchFileManagerTest {
@Test
public void testWatchFile() throws IOException, InterruptedException {
String file = WatchFileManagerTest.class.getResource("/configuration.properties").getFile();
- File f = new File(file);
+ File configFile = new File(file);
File tempConfigFile = new File(tempConfigDir, "configuration.properties");
- Files.copy(f.toPath(), tempConfigFile.toPath());
-
- final FileChangeListener fileChangeListener = new FileChangeListener() {
+ Files.copy(configFile.toPath(), tempConfigFile.toPath());
- @Override
- public void onChanged(FileChangeContext changeContext) {
- Assertions.assertEquals(tempConfigFile.getName(), changeContext.getFileName());
- Assertions.assertEquals(tempConfigFile.getParent(), changeContext.getDirectoryPath());
- }
+ final FileChangeListener mockFileChangeListener = Mockito.mock(FileChangeListener.class);
+ Mockito.when(mockFileChangeListener.support(
+ Mockito.argThat(isFileUnderTest(tempConfigFile.getParent(), tempConfigFile.getName())))
+ ).thenReturn(true);
- @Override
- public boolean support(FileChangeContext changeContext) {
- return changeContext.getWatchEvent().context().toString().contains(tempConfigFile.getName());
- }
- };
- WatchFileManager.registerFileChangeListener(tempConfigFile.getParent(), fileChangeListener);
+ WatchFileManager.registerFileChangeListener(tempConfigFile.getParent(), mockFileChangeListener);
Properties properties = new Properties();
- properties.load(new BufferedReader(new FileReader(tempConfigFile)));
- properties.setProperty("eventMesh.server.newAdd", "newAdd");
- FileWriter fw = new FileWriter(tempConfigFile);
- properties.store(fw, "newAdd");
+ try (
+ BufferedReader bufferedReader = new BufferedReader(new FileReader(configFile));
+ FileWriter fw = new FileWriter(tempConfigFile)
+ ) {
+ properties.load(bufferedReader);
+ properties.setProperty("eventMesh.server.newAdd", "newAdd");
+ properties.store(fw, "newAdd");
+ }
+
+ Mockito.verify(mockFileChangeListener, Mockito.timeout(15_000))
Review Comment:
I bet it is a timing thing. Creating the `FileWriter` actually clears the file, so depending on when the `WatchService` begins monitoring the file, it may pick that up as well, resulting in two calls to `onChanged()`. I think it makes sense to verify it's called at least once, like you suggest.
--
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: issues-unsubscribe@eventmesh.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@eventmesh.apache.org
For additional commands, e-mail: issues-help@eventmesh.apache.org