You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@eventmesh.apache.org by "Pil0tXia (via GitHub)" <gi...@apache.org> on 2023/08/09 17:07:31 UTC
[PR] [ISSUE #4341] WebHookConfig failed to load after being inserted (eventmesh)
Pil0tXia opened a new pull request, #4344:
URL: https://github.com/apache/eventmesh/pull/4344
<!--
### 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 #4341.
### Feature Description
When creating a new WebHookConfig, the `writeToFile` method is invoked to create a file and write data. The `fileWatchRegister` can monitor file creation, modification, and deletion. It calls the `cacheInit` method during creation and modification events to store the WebHookConfig in `cacheWebHookConfig`.
### Bug Phenomenon
Under normal circumstances, when copying the `webhook.github.eventmesh.all` configuration file, both ENTRY_CREATE and ENTRY_MODIFY events are generated as expected:
The bug occurs when calling the `insertWebHookConfig` endpoint for the first time to create a WebHookConfig. Only the ENTRY_CREATE event is captured (abnormal; expected behavior is to have both events):
At this point, the file is empty, and the `cacheInit` method writes null to `cacheWebHookConfig`:
However, the loop ends after one iteration, and `fileWatchRegister` does not call the `cacheInit` method again after the file has been written with WebHookConfig data. Consequently, `cacheWebHookConfig` does not have a valid value, and using the `queryWebHookConfigById` endpoint will not retrieve the WebHookConfig.
### Initial Solution
I resolved this bug by adding a slight delay after capturing the file event:
```java
try {
assert service != null;
key = service.take();
Thread.sleep(1000);
} catch (InterruptedException e) {
log.error("Interrupted", e);
}
```
With this delay, both ENTRY_CREATE and ENTRY_MODIFY events are captured. The `for (final WatchEvent<?> event : key.pollEvents())` loop iterates twice, allowing complete file data reading and the correct creation of WebHookConfig. This behavior is correct and matches expectations.
### Bug Cause
The bug arises when the JVM retrieves file system events faster than file write speed. In such cases, the obtained file events and data are incomplete. When the JVM retrieves file system events slower than file write speed, the events and data are complete.
In other words, when `fileWatchRegister` captures a file creation event, without adding a delay, `key.pollEvents()` will return only an ENTRY_CREATE event, not waiting long enough for the file's content to be fully written to disk.
File write speed is beyond control, and a fixed delay isn't a robust solution. Interestingly, I found that this bug doesn't fully reproduce on a different machine.
### Better Solution
My code should be more robust. A better solution is to ensure the file is fully created and written before executing actions. Therefore, I listen to the "ENTRY_MODIFY" event of the file rather than performing `cacheInit(file)` in the "ENTRY_CREATE" event. This approach provides more certainty in waiting for complete file writes.
### Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not 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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on code in PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#discussion_r1296505244
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -173,18 +175,25 @@ private WebHookConfig getWebHookConfigFromFile(final File webhookConfigFile) {
}
public static boolean writeToFile(final File webhookConfigFile, final WebHookConfig webHookConfig) {
- try (FileOutputStream fos = new FileOutputStream(webhookConfigFile);
- BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(fos, StandardCharsets.UTF_8))) {
- // lock this file, and will auto release after fos close
- fos.getChannel().lock();
- bw.write(Objects.requireNonNull(JsonUtils.toJSONString(webHookConfig)));
- } catch (IOException e) {
- if (log.isErrorEnabled()) {
- log.error("write webhookConfig {} to file error", webHookConfig.getCallbackPath());
+ // Wait for the previous cacheInit to complete and ensure the atomicity of countDown in case of concurrency
+ synchronized (SharedLatchHolder.lock) {
+ // Reset latch count
+ SharedLatchHolder.latch = new CountDownLatch(1);
Review Comment:
```
SharedLatchHolder.latch.await();
synchronized (SharedLatchHolder.lock) {
cacheInit(file);
}
```
Now `SharedLatchHolder.latch` seems redundant.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on code in PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#discussion_r1295741848
##########
eventmesh-webhook/eventmesh-webhook-api/src/main/java/org/apache/eventmesh/webhook/api/common/SharedLatchHolder.java:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.eventmesh.webhook.api.common;
+
+import java.util.concurrent.CountDownLatch;
+
+public class SharedLatchHolder {
+ public static CountDownLatch latch = new CountDownLatch(1);
Review Comment:
writeToFile and cacheInit are located in two different modules, so if we pass CountDownLatch as a parameter in the method, we need to instantiate CountDownLatch in `EventMeshServer`, which is obviously not appropriate. In the SharedLatchHolder, only the writeToFile and cacheInit methods use CountDownLatch, so it's a safe shared lock.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on code in PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#discussion_r1297012782
##########
eventmesh-webhook/eventmesh-webhook-api/src/main/java/org/apache/eventmesh/webhook/api/common/SharedLatchHolder.java:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.eventmesh.webhook.api.common;
+
+import java.util.concurrent.CountDownLatch;
+
+public class SharedLatchHolder {
+ public static CountDownLatch latch = new CountDownLatch(1);
Review Comment:
> Don't worry too much about it. We have reviewers
I think this comment is a bit worrying. It will mislead contributors to no longer pay attention to the quality of their codes, and rely more on reviewers to improve them.
> and we can also add comments in similar places to make convention outweigh implementation.
"Convention Over Configuration" in open-source frameworks is not in this way. It involves providing default values, default code structures, templates, and similar forms that shield the underlying complexity, simplify development, and keep flexibility. Instead of exposing code that should not be exposed and then accompanied by "prohibition of use" instructions.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on code in PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#discussion_r1296505244
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -173,18 +175,25 @@ private WebHookConfig getWebHookConfigFromFile(final File webhookConfigFile) {
}
public static boolean writeToFile(final File webhookConfigFile, final WebHookConfig webHookConfig) {
- try (FileOutputStream fos = new FileOutputStream(webhookConfigFile);
- BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(fos, StandardCharsets.UTF_8))) {
- // lock this file, and will auto release after fos close
- fos.getChannel().lock();
- bw.write(Objects.requireNonNull(JsonUtils.toJSONString(webHookConfig)));
- } catch (IOException e) {
- if (log.isErrorEnabled()) {
- log.error("write webhookConfig {} to file error", webHookConfig.getCallbackPath());
+ // Wait for the previous cacheInit to complete and ensure the atomicity of countDown in case of concurrency
+ synchronized (SharedLatchHolder.lock) {
+ // Reset latch count
+ SharedLatchHolder.latch = new CountDownLatch(1);
Review Comment:
https://github.com/apache/eventmesh/blob/5005cada8c66193acbff2a278ca3f8bca117de37/eventmesh-webhook/eventmesh-webhook-receive/src/main/java/org/apache/eventmesh/webhook/receive/storage/WebhookFileListener.java#L173-L176
Now `SharedLatchHolder.latch` seems redundant.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on code in PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#discussion_r1295780269
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -173,9 +175,11 @@ private WebHookConfig getWebHookConfigFromFile(final File webhookConfigFile) {
}
public static boolean writeToFile(final File webhookConfigFile, final WebHookConfig webHookConfig) {
+ // Reset latch count
+ SharedLatchHolder.latch = new CountDownLatch(1);
Review Comment:
> that doesn't seem to be used concurrently.
Are you sure about that? `insertWebHookConfig` and `updateWebHookConfig` are the same situation.
Also, regardless of the situation here, considering the next reviewing, I don't think this is OK.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on code in PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#discussion_r1295788383
##########
eventmesh-webhook/eventmesh-webhook-api/src/main/java/org/apache/eventmesh/webhook/api/common/SharedLatchHolder.java:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.eventmesh.webhook.api.common;
+
+import java.util.concurrent.CountDownLatch;
+
+public class SharedLatchHolder {
+ public static CountDownLatch latch = new CountDownLatch(1);
Review Comment:
> only the writeToFile and cacheInit methods use CountDownLatch
It is "only the writeToFile and cacheInit methods use CountDownLatch for now". There is nothing to prohibit the use of `SharedLatchHolder` elsewhere in the future.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on code in PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#discussion_r1296674504
##########
eventmesh-webhook/eventmesh-webhook-api/src/main/java/org/apache/eventmesh/webhook/api/common/SharedLatchHolder.java:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.eventmesh.webhook.api.common;
+
+import java.util.concurrent.CountDownLatch;
+
+public class SharedLatchHolder {
+ public static CountDownLatch latch = new CountDownLatch(1);
Review Comment:
Reasonable and done. Don't worry too much about it. We have reviewers, and we can also add comments in similar places to make convention outweigh implementation.
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -173,18 +175,25 @@ private WebHookConfig getWebHookConfigFromFile(final File webhookConfigFile) {
}
public static boolean writeToFile(final File webhookConfigFile, final WebHookConfig webHookConfig) {
- try (FileOutputStream fos = new FileOutputStream(webhookConfigFile);
- BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(fos, StandardCharsets.UTF_8))) {
- // lock this file, and will auto release after fos close
- fos.getChannel().lock();
- bw.write(Objects.requireNonNull(JsonUtils.toJSONString(webHookConfig)));
- } catch (IOException e) {
- if (log.isErrorEnabled()) {
- log.error("write webhookConfig {} to file error", webHookConfig.getCallbackPath());
+ // Wait for the previous cacheInit to complete and ensure the atomicity of countDown in case of concurrency
+ synchronized (SharedLatchHolder.lock) {
+ // Reset latch count
+ SharedLatchHolder.latch = new CountDownLatch(1);
Review Comment:
Reasonable. The ENTRY_MODIFY event is triggered inside `writeToFile` and `cacheInit` will wait for write completion.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on code in PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#discussion_r1295780269
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -173,9 +175,11 @@ private WebHookConfig getWebHookConfigFromFile(final File webhookConfigFile) {
}
public static boolean writeToFile(final File webhookConfigFile, final WebHookConfig webHookConfig) {
+ // Reset latch count
+ SharedLatchHolder.latch = new CountDownLatch(1);
Review Comment:
> that doesn't seem to be used concurrently.
Are you sure about that? `insertWebHookConfig` and `updateWebHookConfig` are the same situation.
If so, I have no objections. If not, I think it's not OK.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "Alonexc (via GitHub)" <gi...@apache.org>.
Alonexc commented on PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#issuecomment-1672487163
The problem can be solved by merging the master branch.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on code in PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#discussion_r1295730208
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -173,9 +175,11 @@ private WebHookConfig getWebHookConfigFromFile(final File webhookConfigFile) {
}
public static boolean writeToFile(final File webhookConfigFile, final WebHookConfig webHookConfig) {
+ // Reset latch count
+ SharedLatchHolder.latch = new CountDownLatch(1);
Review Comment:
Thank you for pointing out this problem. `insertWebHookConfig` is a configuration endpoint that doesn't seem to be used concurrently. Also, this problem can be resolved by using the synchronized keyword.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#issuecomment-1681615940
I have fixed a new bug: When the `updateWebHookConfig` endpoint is called with high concurrency and the `deleteWebHookConfig` endpoint is also called, if the `cacheInit` method hasn't completed yet and the webhook configuration file is deleted, the `cacheInit` method will throw a `java.nio.file.AccessDeniedException`. A more common scenario is that after passing the `file.isFile()` validation, the `file` is deleted before the `cacheInit` method is called, causing the `cacheInit` method to throw a `java.nio.file.NoSuchFileException`.
After the fix, `file.isFile()` and `cacheInit(file)` will become an atomic operation, ensuring the validity of the configuration file. This approach ensures the eventual consistency of the data, and introducing a task queue can further guarantee the sequential consistency of the data. Given the limited concurrency of this endpoint, the current synchronization mechanism is already fully sufficient.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on code in PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#discussion_r1295780269
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -173,9 +175,11 @@ private WebHookConfig getWebHookConfigFromFile(final File webhookConfigFile) {
}
public static boolean writeToFile(final File webhookConfigFile, final WebHookConfig webHookConfig) {
+ // Reset latch count
+ SharedLatchHolder.latch = new CountDownLatch(1);
Review Comment:
> that doesn't seem to be used concurrently.
Are you sure about that? `insertWebHookConfig` and `updateWebHookConfig` are the same situation.
Additionally, regardless of the situation here, considering the next reviewing, I don't think this is OK.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#issuecomment-1689797336
@xwm1992 ping~
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "xwm1992 (via GitHub)" <gi...@apache.org>.
xwm1992 merged PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "pandaapo (via GitHub)" <gi...@apache.org>.
pandaapo commented on code in PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#discussion_r1295554952
##########
eventmesh-webhook/eventmesh-webhook-admin/src/main/java/org/apache/eventmesh/webhook/admin/FileWebHookConfigOperation.java:
##########
@@ -173,9 +175,11 @@ private WebHookConfig getWebHookConfigFromFile(final File webhookConfigFile) {
}
public static boolean writeToFile(final File webhookConfigFile, final WebHookConfig webHookConfig) {
+ // Reset latch count
+ SharedLatchHolder.latch = new CountDownLatch(1);
Review Comment:
When concurrent threads call this method, `SharedLatchHolder.latch` will be overwritten by one of the threads. Therefore, will the lock object of the `SharedLatchHolder.latch.await()` in the `WebhookFileListener` be uncertain?
##########
eventmesh-webhook/eventmesh-webhook-api/src/main/java/org/apache/eventmesh/webhook/api/common/SharedLatchHolder.java:
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.eventmesh.webhook.api.common;
+
+import java.util.concurrent.CountDownLatch;
+
+public class SharedLatchHolder {
+ public static CountDownLatch latch = new CountDownLatch(1);
Review Comment:
A static `CountDownLatch` field can be assigned and released anywhere. It does not seem to be an appropriate way to use `CountDownLatch`.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#issuecomment-1673594283
I have resolved a new issue of `master` branch that occurred when calling the `updateWebHookConfig` endpoint, which resulted in an NPE:
I couldn't reproduce this NPE in debug mode. After adding a few simple logs, I found that the `fileWatchRegister` obtained an empty file. Each endpoint request triggered two file modification events, resulting in the first occurrence of an NPE and the second occurrence functioning normally:
Still, the issue is resolved by simply adding a delay before retrieving the events. With two endpoint requests, each request triggers only one file modification event:
Detailed logs of file locking, file acquisition, caching, and file modification events are shown. After the file is written, the second file modification event is triggered, which works normally:
You can clearly observe that after calling the `updateWebHookConfig` endpoint, a file modification event is triggered when creating a `FileOutputStream` in the `writeToFile` function. In this event, `fileWatchRegister` obtains an empty file before locking and writing to it, and then `cacheInit` is called to write `null` to `cacheWebHookConfig`, resulting in the NPE:
As seen, the NPE is thrown before the file locking is completed, so the `tryLock` method cannot be used to determine the file status.
Previously, when inserting configurations, both ENTRY_CREATE and ENTRY_MODIFY events would occur, allowing differentiation based on the event type. However, when updating configurations, only an ENTRY_MODIFY event is generated, which is distinct from the former. Therefore, I used `CountDownLatch` to notify when the file write is completed and to wait for the notification before initializing the cache, which resolves the issue.
--
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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#issuecomment-1674134915
## [Codecov](https://app.codecov.io/gh/apache/eventmesh/pull/4344?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#4344](https://app.codecov.io/gh/apache/eventmesh/pull/4344?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8ae7a39) into [master](https://app.codecov.io/gh/apache/eventmesh/commit/d3d9d4ffd3881e03a8a5965b4a35f2daaca0949a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (d3d9d4f) will **decrease** coverage by `0.01%`.
> Report is 1 commits behind head on master.
> The diff coverage is `17.39%`.
> :exclamation: Current head 8ae7a39 differs from pull request most recent head 3d1b850. Consider uploading reports for the commit 3d1b850 to get more accurate results
```diff
@@ Coverage Diff @@
## master #4344 +/- ##
============================================
- Coverage 16.69% 16.69% -0.01%
Complexity 1393 1393
============================================
Files 594 594
Lines 25348 25356 +8
Branches 2393 2379 -14
============================================
+ Hits 4233 4234 +1
- Misses 20685 20693 +8
+ Partials 430 429 -1
```
| [Files Changed](https://app.codecov.io/gh/apache/eventmesh/pull/4344?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...h/webhook/receive/storage/WebhookFileListener.java](https://app.codecov.io/gh/apache/eventmesh/pull/4344?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXZlbnRtZXNoLXdlYmhvb2svZXZlbnRtZXNoLXdlYmhvb2stcmVjZWl2ZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZXZlbnRtZXNoL3dlYmhvb2svcmVjZWl2ZS9zdG9yYWdlL1dlYmhvb2tGaWxlTGlzdGVuZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...mesh/webhook/admin/FileWebHookConfigOperation.java](https://app.codecov.io/gh/apache/eventmesh/pull/4344?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXZlbnRtZXNoLXdlYmhvb2svZXZlbnRtZXNoLXdlYmhvb2stYWRtaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2V2ZW50bWVzaC93ZWJob29rL2FkbWluL0ZpbGVXZWJIb29rQ29uZmlnT3BlcmF0aW9uLmphdmE=) | `44.31% <40.00%> (+1.10%)` | :arrow_up: |
... and [7 files with indirect coverage changes](https://app.codecov.io/gh/apache/eventmesh/pull/4344/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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 #4341] WebHookConfig failed to load after being inserted (eventmesh)
Posted by "Pil0tXia (via GitHub)" <gi...@apache.org>.
Pil0tXia commented on PR #4344:
URL: https://github.com/apache/eventmesh/pull/4344#issuecomment-1672670013
@Alonexc done~
--
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