Skip to content
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

Issue #400: Prevent side effects / glitches in Observable implementations. #453

Closed

Conversation

@pocmo
Copy link
Contributor

@pocmo pocmo commented Jul 19, 2018

This is a first version of a helper to fix the issue described in #400.

It's not used in any Observable yet. We may want to use it in SessionManager and Session but probably not EngineSession implementations because observers do not really modify the internal state.

@PublishedApi internal val queue: MutableList<() -> Unit> = mutableListOf()
@PublishedApi internal var isProcessing = false

inline fun modifyWithoutSideEffects(crossinline action: () -> Unit): Unit = synchronized(lock) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ktlint] WARNING: [no-unit-return] Unnecessary "Unit" return type in column 74

Loading

@codecov
Copy link

@codecov codecov bot commented Jul 19, 2018

Codecov Report

Merging #453 into master will decrease coverage by 0.88%.
The diff coverage is 25%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #453      +/-   ##
============================================
- Coverage     76.54%   75.65%   -0.89%     
+ Complexity      915      898      -17     
============================================
  Files           143      137       -6     
  Lines          3376     3282      -94     
  Branches        484      462      -22     
============================================
- Hits           2584     2483     -101     
- Misses          544      560      +16     
+ Partials        248      239       -9
Impacted Files Coverage Δ Complexity Δ
...a/components/support/utils/observer/SideEffects.kt 25% <25%> (ø) 4 <4> (?)
...omponents/service/fretboard/ExperimentEvaluator.kt 73.17% <0%> (-12.88%) 26% <0%> (ø)
.../mozilla/components/service/fretboard/Fretboard.kt 61.11% <0%> (-8.89%) 4% <0%> (-6%)
...illa/telemetry/ping/TelemetryEventPingBuilder.java 92.3% <0%> (-7.7%) 4% <0%> (-1%)
...omponents/browser/session/engine/EngineObserver.kt 63.63% <0%> (-5.6%) 8% <0%> (-1%)
...ce/fretboard/source/kinto/KintoExperimentSource.kt 76.47% <0%> (-3.53%) 17% <0%> (+5%)
...retboard/storage/flatfile/ExperimentsSerializer.kt 93.33% <0%> (-1.67%) 5% <0%> (-1%)
...ents/ui/autocomplete/InlineAutocompleteEditText.kt 72.47% <0%> (-1.26%) 43% <0%> (ø)
...src/main/java/org/mozilla/telemetry/Telemetry.java 68.83% <0%> (-0.59%) 14% <0%> (-1%)
.../mozilla/components/browser/search/SearchEngine.kt 100% <0%> (ø) 7% <0%> (-5%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f885ae3...d89db2d. Read the comment docs.

Loading

@pocmo
Copy link
Contributor Author

@pocmo pocmo commented Jul 20, 2018

@csadilek What do you think? If you agree I'll go ahead with the patch and apply it to SessionManager and Session.

Loading


queue.forEach { action ->
action()
}
Copy link
Contributor Author

@pocmo pocmo Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may cause a concurrent modification exception if we are adding something to the queue while processing the queue. I may add a test for that and fix it. We may want to use an actual queue here.

Loading

Copy link
Contributor

@csadilek csadilek Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm yes, or we take the recursive approach: #400 (comment) ?

Loading

} else {
isProcessing = true

action()
Copy link
Contributor Author

@pocmo pocmo Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid that a return statement inside the lambda will jump out of this method because it's getting inlined. I thought crossinline would prevent that. But I seem to be able to still use a return@label:

fun someMethod(bar: Foo?) = sideEffects.modifyWithoutSideEffects {
    if (bar == null) {
        // This doesn't compile because of 'crossinline'
        return
    }

    if (bar == null) {
        // This compiles... but isn't it the same as the one above?
        return@modifyWithoutSideEffects
    }

    // ...
}

Loading

Copy link
Contributor Author

@pocmo pocmo Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if every action() should actually be wrapped in:

try {
    action()
} finally {
   processQueue()
}

... so that we will always continue with the next action if one of them returns after being inlined?

Loading

Copy link
Contributor Author

@pocmo pocmo Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This try catch doesn't really work... Every action we process in the queue would need to be wrapped in the same try-catch. We can do that with recursion - but we can't inline a method that is called recursively..

Loading

Copy link
Contributor

@csadilek csadilek Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Shouldn't we just use noinline for the lambda here? It shouldn't have the power to jump out of the executing method :).

Loading

Copy link
Contributor Author

@pocmo pocmo Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to inline aggressively so that using SideEffects has no penalty. Otherwise Java (6) will need to create an object (+ the class) for every lambda :(

Loading

@csadilek
Copy link
Contributor

@csadilek csadilek commented Jul 20, 2018

@pocmo yes, I like the solution. This way the observable can "decide" whether or not it allows side-effects. So, let's get in hardened and landed ;).

Loading

@pocmo pocmo force-pushed the observers-without-sideeffects branch from 9bdd9ac to ef57ba7 Jul 25, 2018
@pocmo pocmo force-pushed the observers-without-sideeffects branch from ef57ba7 to d89db2d Jul 25, 2018
@pocmo
Copy link
Contributor Author

@pocmo pocmo commented Jul 25, 2018

Updated the PR.

This is working nicely with SessionManager now. I just wanted to apply it to Session and then realized that we are using Delegates.observable here. Queuing up the change here isn't as straight-forward.

Loading

@pocmo
Copy link
Contributor Author

@pocmo pocmo commented Jul 25, 2018

Unable to instrument file with Jacoco: /home/travis/build/mozilla-mobile/android-components/components/browser/session/build/tmp/kotlin-classes/debug/mozilla/components/browser/session/SessionManager$removeSessions$$inlined$modifyWithoutSideEffects$1.class

🔥

Loading

@pocmo
Copy link
Contributor Author

@pocmo pocmo commented Jul 26, 2018

Closing: I moved this PR out of the sprint. I want to think a bit more about this.

Loading

@pocmo pocmo closed this Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants