You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@felix.apache.org by GitBox <gi...@apache.org> on 2022/08/05 10:49:46 UTC
[GitHub] [felix-dev] amitjoy opened a new pull request, #170: [FELIX-6554] Fixed concurrent access to the internal maps
amitjoy opened a new pull request, #170:
URL: https://github.com/apache/felix-dev/pull/170
Since the bundles can appear/disappear dynamically, concurrent access is required for these maps
Fixes [FELIX-6554](https://issues.apache.org/jira/browse/FELIX-6554)
--
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@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on a diff in pull request #170: [FELIX-6554] Fixed concurrent access to the internal maps
Posted by GitBox <gi...@apache.org>.
amitjoy commented on code in PR #170:
URL: https://github.com/apache/felix-dev/pull/170#discussion_r939998000
##########
configurator/src/main/java/org/apache/felix/configurator/impl/model/State.java:
##########
@@ -39,9 +39,8 @@ public class State extends AbstractState implements Serializable {
public static final String FILE_NAME = "state.ser";
- private Map<Long, Long> bundlesLastModified = new HashMap<>();
-
- private Map<Long, Long> bundlesConfigAdminBundleId = new HashMap<>();
+ private Map<Long, Long> bundlesLastModified = new ConcurrentHashMap<>();
Review Comment:
@tjwatson @cziegeler I have updated the PR. Could you kindly have another look?
--
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@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] cziegeler commented on a diff in pull request #170: [FELIX-6554] Fixed concurrent access to the internal maps
Posted by GitBox <gi...@apache.org>.
cziegeler commented on code in PR #170:
URL: https://github.com/apache/felix-dev/pull/170#discussion_r939535509
##########
configurator/src/main/java/org/apache/felix/configurator/impl/model/State.java:
##########
@@ -39,9 +39,8 @@ public class State extends AbstractState implements Serializable {
public static final String FILE_NAME = "state.ser";
- private Map<Long, Long> bundlesLastModified = new HashMap<>();
-
- private Map<Long, Long> bundlesConfigAdminBundleId = new HashMap<>();
+ private Map<Long, Long> bundlesLastModified = new ConcurrentHashMap<>();
Review Comment:
Tom is right, you need to create the right type on deseralization.
But, it might also be that the synchronization is on the wrong level and instead of making the properties of this class thread safe take care of syncing on a higher level. I think, that was the intention, but I can't really remember the details
--
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@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on pull request #170: [FELIX-6554] Fixed concurrent access to the internal maps
Posted by GitBox <gi...@apache.org>.
amitjoy commented on PR #170:
URL: https://github.com/apache/felix-dev/pull/170#issuecomment-1206343075
@cziegeler @tjwatson Could you kindly have a look?
--
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@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] cziegeler commented on pull request #170: [FELIX-6554] Fixed concurrent access to the internal data
Posted by GitBox <gi...@apache.org>.
cziegeler commented on PR #170:
URL: https://github.com/apache/felix-dev/pull/170#issuecomment-1208008093
Thanks @amitjoy - the PR by itself looks good now; but it is probably fixing the problem at the wrong place, see https://issues.apache.org/jira/browse/FELIX-6554
--
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@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] tjwatson commented on a diff in pull request #170: [FELIX-6554] Fixed concurrent access to the internal maps
Posted by GitBox <gi...@apache.org>.
tjwatson commented on code in PR #170:
URL: https://github.com/apache/felix-dev/pull/170#discussion_r938748646
##########
configurator/src/main/java/org/apache/felix/configurator/impl/model/State.java:
##########
@@ -39,9 +39,8 @@ public class State extends AbstractState implements Serializable {
public static final String FILE_NAME = "state.ser";
- private Map<Long, Long> bundlesLastModified = new HashMap<>();
-
- private Map<Long, Long> bundlesConfigAdminBundleId = new HashMap<>();
+ private Map<Long, Long> bundlesLastModified = new ConcurrentHashMap<>();
Review Comment:
I don't get why the default implementation of serializable is not used here to read/write the maps so you can make these final fields.
--
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@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on pull request #170: [FELIX-6554] Fixed concurrent access to the internal data
Posted by GitBox <gi...@apache.org>.
amitjoy commented on PR #170:
URL: https://github.com/apache/felix-dev/pull/170#issuecomment-1209157238
The fix has already been introduced, therefore, closing it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy closed pull request #170: [FELIX-6554] Fixed concurrent access to the internal data
Posted by GitBox <gi...@apache.org>.
amitjoy closed pull request #170: [FELIX-6554] Fixed concurrent access to the internal data
URL: https://github.com/apache/felix-dev/pull/170
--
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@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] cziegeler commented on a diff in pull request #170: [FELIX-6554] Fixed concurrent access to the internal maps
Posted by GitBox <gi...@apache.org>.
cziegeler commented on code in PR #170:
URL: https://github.com/apache/felix-dev/pull/170#discussion_r938778902
##########
configurator/src/main/java/org/apache/felix/configurator/impl/model/State.java:
##########
@@ -39,9 +39,8 @@ public class State extends AbstractState implements Serializable {
public static final String FILE_NAME = "state.ser";
- private Map<Long, Long> bundlesLastModified = new HashMap<>();
-
- private Map<Long, Long> bundlesConfigAdminBundleId = new HashMap<>();
+ private Map<Long, Long> bundlesLastModified = new ConcurrentHashMap<>();
Review Comment:
Its used for versioning of the serialized format, it makes it easier to make any change to this class in future versions. It might be a little bit overkill here
But at least it allows to change the type of the map when reading already serialized objects - and that's what I think is missing in this PR
--
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@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] cziegeler commented on a diff in pull request #170: [FELIX-6554] Fixed concurrent access to the internal maps
Posted by GitBox <gi...@apache.org>.
cziegeler commented on code in PR #170:
URL: https://github.com/apache/felix-dev/pull/170#discussion_r938778902
##########
configurator/src/main/java/org/apache/felix/configurator/impl/model/State.java:
##########
@@ -39,9 +39,8 @@ public class State extends AbstractState implements Serializable {
public static final String FILE_NAME = "state.ser";
- private Map<Long, Long> bundlesLastModified = new HashMap<>();
-
- private Map<Long, Long> bundlesConfigAdminBundleId = new HashMap<>();
+ private Map<Long, Long> bundlesLastModified = new ConcurrentHashMap<>();
Review Comment:
Its used for versioning of the serialized format, it makes it easier to make any change to this class in future versions. It might be a little bit overkill 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: dev-unsubscribe@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on a diff in pull request #170: [FELIX-6554] Fixed concurrent access to the internal maps
Posted by GitBox <gi...@apache.org>.
amitjoy commented on code in PR #170:
URL: https://github.com/apache/felix-dev/pull/170#discussion_r938877322
##########
configurator/src/main/java/org/apache/felix/configurator/impl/model/State.java:
##########
@@ -39,9 +39,8 @@ public class State extends AbstractState implements Serializable {
public static final String FILE_NAME = "state.ser";
- private Map<Long, Long> bundlesLastModified = new HashMap<>();
-
- private Map<Long, Long> bundlesConfigAdminBundleId = new HashMap<>();
+ private Map<Long, Long> bundlesLastModified = new ConcurrentHashMap<>();
Review Comment:
@cziegeler Could you kindly elaborate what I am missing from the PR?
--
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@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] tjwatson commented on a diff in pull request #170: [FELIX-6554] Fixed concurrent access to the internal maps
Posted by GitBox <gi...@apache.org>.
tjwatson commented on code in PR #170:
URL: https://github.com/apache/felix-dev/pull/170#discussion_r938933632
##########
configurator/src/main/java/org/apache/felix/configurator/impl/model/State.java:
##########
@@ -39,9 +39,8 @@ public class State extends AbstractState implements Serializable {
public static final String FILE_NAME = "state.ser";
- private Map<Long, Long> bundlesLastModified = new HashMap<>();
-
- private Map<Long, Long> bundlesConfigAdminBundleId = new HashMap<>();
+ private Map<Long, Long> bundlesLastModified = new ConcurrentHashMap<>();
Review Comment:
The Map type changing is what triggered my comment about serialization. The concern is that a previous data stream would be storing a HashMap instance and when you do the read on restoring it will construct a HashMap instance back. So in that case you need to read into a Map local instance for the call to `Map<Long, Long>) in.readObject();` then check the instance of the Map. If it is not a ConcurrentHashMap then you need to copy the content of the read map into a new ConcurrentHashMap instance.
But it seems there is other issues with thread safety for the `private volatile Set<String> initialHashes` set. Is that always set to a ConcurrentHashSet or something similar to ensure it is thread safe?
--
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@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [felix-dev] amitjoy commented on a diff in pull request #170: [FELIX-6554] Fixed concurrent access to the internal maps
Posted by GitBox <gi...@apache.org>.
amitjoy commented on code in PR #170:
URL: https://github.com/apache/felix-dev/pull/170#discussion_r939933209
##########
configurator/src/main/java/org/apache/felix/configurator/impl/model/State.java:
##########
@@ -39,9 +39,8 @@ public class State extends AbstractState implements Serializable {
public static final String FILE_NAME = "state.ser";
- private Map<Long, Long> bundlesLastModified = new HashMap<>();
-
- private Map<Long, Long> bundlesConfigAdminBundleId = new HashMap<>();
+ private Map<Long, Long> bundlesLastModified = new ConcurrentHashMap<>();
Review Comment:
@tjwatson @cziegeler Thanks a lot for the clarifications 👍 It's my bad since I didn't notice it earlier. I will add the necessary changes and update the PR accordingly.
--
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@felix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org