This repository has been archived by the owner on Feb 8, 2019. It is now read-only.
Fixes #10677: Datasources plugin call continuously datasource url even after delete #23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fanf
force-pushed
the
bug_10677/datasources_plugin_call_continuously_datasource_url_even_after_delete
branch
from
28d0c23
to
e6758c1
Compare
|
PR rebased |
fanf
commented
Jun 27, 2017
| @@ -199,8 +215,6 @@ class DataSourceRepoImpl( | |||
|
|
|||
| } | |||
| private[this] def updateDataSourceScheduler(source: DataSource, delay: Option[FiniteDuration]): Unit = { | |||
There was a problem hiding this comment.
So, the problem was most likely here: that method is not synchronized, and was used on non sync method. So what could have happened:
[context $1]
- stop source.id $0,
[switch context to $2] - stop source.id $0,
- create dss $2
- save dss $2
- start dss $2
[switch context $1] - create dss $1
- save dss $1
- start dss $1
So now, both $1 and $2 run, but only $1 is know for rudder. This is bad.
ncharles
reviewed
Jun 27, 2017
| @@ -242,7 +256,7 @@ class DataSourceRepoImpl( | |||
| * on update, we need to stop the corresponding optionnaly existing | |||
| * scheduler, and update with the new one. | |||
| */ | |||
| override def save(source : DataSource) : Box[DataSource] = synchronized { | |||
| override def save(source : DataSource) : Box[DataSource] = { | |||
There was a problem hiding this comment.
i think it shold stay synchronized (but not 100% sure)
fanf
force-pushed
the
bug_10677/datasources_plugin_call_continuously_datasource_url_even_after_delete
branch
from
e6758c1
to
5717e0e
Compare
|
PR rebased |
1 similar comment
|
PR rebased |
fanf
force-pushed
the
bug_10677/datasources_plugin_call_continuously_datasource_url_even_after_delete
branch
from
5717e0e
to
065cc78
Compare
ncharles
reviewed
Jun 27, 2017
| private[this] def stop(id: DataSourceId) = { | ||
| DataSourceLogger.debug(s"Stopping data source with id '${id.value}'") | ||
| internal.get(id) match { | ||
| case None => s"Data source with id ${id.value} was not found running" |
There was a problem hiding this comment.
this text doesn't go anywhere, unless i'm mistaken. Shouldn't it go to a logger ?
fanf
force-pushed
the
bug_10677/datasources_plugin_call_continuously_datasource_url_even_after_delete
branch
from
065cc78
to
09ecda3
Compare
|
PR rebased |
|
OK, merging this PR |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://www.rudder-project.org/redmine/issues/10677
That pull request correct a nasty race condition. I was not able to reproduce it live, but the race is clearly there.
I could just have added synchronized on updateDataSourceScheduler, but it would have been bugs wainting to flourish, so I prefered to encapsulate all the must-be synchronized methods dealing with our mutable var in a synchronized object.
To sum up: mutable var are bad. Mutable vars combined with threads are a can of worms, the worms being able to fly and kill any life on a touch. And yes, you have to unknot the thing.