New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concurrency Issues in StateRepositories #21

Closed
jadlr opened this Issue Mar 3, 2013 · 10 comments

Comments

Projects
None yet
2 participants
@jadlr

jadlr commented Mar 3, 2013

Hi,

I noticed a concurrency bug in the FileBasedStateRepository (writes are not synced) and while implementing a fix for it I noticed that none of the StateRepositories (besides the JDBC Repo) are thread safe, so I thought I'd ask if this is by design? Since you provide a servlet that lets you change the state of StateRepos that are at the same time used by code that leverages togglz I think it would be best to make the StateRepositories thread safe.

In the case of the FileBasedStateRepository this is especially dangerous because the file could be written outside of the JVM while it is written/read by your code. A solution that would depend on the architecture the JVM runs on would be to acquire a FileLock before writing to/reading from it.

Let me know what you think,

jadlr

@chkal

This comment has been minimized.

Member

chkal commented Mar 4, 2013

Hey jadlr,

thanks a lot for bringing up this issue. Yes, you are completely right. The state repositories should be thread safe. That's a bug that should be fixed.

Would you be interested in helping to fix this? This would be great. Especially as you have already looked into the problem.

I've just released Togglz 2.0.0.Alpha1 which includes some major refactorings. But I think it would make sense to release 1.1.1 based on 1.1.0 to also fix this. Especially because it will take some time to finish Togglz 2.0.0.

@chkal

This comment has been minimized.

Member

chkal commented Mar 4, 2013

I just created a maintenance branch for 1.1.x here:

https://github.com/chkal/togglz/tree/1.1.x

@jadlr

This comment has been minimized.

jadlr commented Mar 4, 2013

i'll get to it once I have a few hours at hand which might take a bit.

@chkal

This comment has been minimized.

Member

chkal commented Mar 4, 2013

Sure, I'll also try to find some time to fix this issue.

@chkal

This comment has been minimized.

Member

chkal commented Mar 5, 2013

Hey jadlr,

I reproduce the concurrency issue with FileBasedStateRepository with a test case. For now I simply fixed it by making getFeatureState() and setFeatureState() synchronized. I know that's not the most elegant solution, but I think it's ok for a 1.1.1.Final release. Everything else should only go into 2.0.0-SNAPSHOT.

What do you think? Will this fix the concurrency issue you ran into?

Christian

@ghost

This comment has been minimized.

ghost commented Mar 5, 2013

wouldn't syncronizing getFeatureState() hurt performance?

@jadlr

This comment has been minimized.

jadlr commented Mar 5, 2013

I think as a quick fix it would suffice to synchronize private void write(Properties newValues)

@chkal

This comment has been minimized.

Member

chkal commented Mar 6, 2013

Thanks for you comments on this.

I also synchronized the getFeatureState() method because this ensures that the state read from the file is consistent. As the method reads multiple properties to obtain the feature's state, it would otherwise be possible that another thread triggers a reload of the file which could (in rare cases) lead to the situation that the feature state is partially read from the old file and partially from the new file. But I don't think that it has a huge performance impact because in most cases the file will not be read at all and the complete operation will happen in memory.

And I synchronized setFeatureState() instead of write(Properties) because writing the a new feature state consists of the following steps:

  1. Get a copy of all the properties in the file
  2. modify the properties as required
  3. Save the modified copy to the file

If two threads execute these three steps concurrently, this could happen: If thread A performs step 2 and a thread B enters steps 1 concurrently and later writes the data after A has written it's date, the update of A gets lost.

I think it would be better to fix concurrency issue with simple synchronized blocks for 1.1.1.Final, even if there will be an (IMHO minor) performance decrease. For 2.0.0.Final we could try to come up with a more performance implementation. But this will take some time (and many tests).

@jadlr jadlr closed this Mar 6, 2013

@jadlr jadlr reopened this Mar 6, 2013

@jadlr

This comment has been minimized.

jadlr commented Mar 6, 2013

You're right, this way it should be safe.

@chkal

This comment has been minimized.

Member

chkal commented Mar 6, 2013

Thanks you very much for reporting this issue. I'll do some more reviews and then release 1.1.1.Final soon.

@chkal chkal closed this Mar 6, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment