Skip to content

Fix dropped errors in manifold.time/in #247

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: detect-dropped-errors-in-tests
Choose a base branch
from
Open

Fix dropped errors in manifold.time/in #247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/manifold/time.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@
;;;

(in-ns 'manifold.deferred)
(clojure.core/declare success! error! deferred realized? chain connect)
(clojure.core/declare success! error! deferred realized? chain connect on-realized)
(in-ns 'manifold.time)

;;;
Expand Down Expand Up @@ -191,8 +191,14 @@
(reify
IClock
(in [this interval-millis f]
(swap! events update-in [(p/+ ^double @now interval-millis)] #(conj (or % []) f))
(advance this 0))
(let [t (p/+ ^double @now interval-millis)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be a swap! ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On now you mean? If so: no, the current clock time can only be changed via advance for the mock clock. Also note that this was already the case before - I merely pulled out the key construction from the update-in so that I can re-use it in cancel-fn.

cancel-fn (fn []
(swap! events #(cond-> %
(contains? % t)
(update t disj f))))]
(swap! events update t (fnil conj #{}) f)
Copy link
Contributor

Choose a reason for hiding this comment

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

could there be a race condition between the deref now on L194 and this swap?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only if there is a concurrent call to advance (which would be an odd thing to do in a test 😅). Even if, it wouldn't be an issue. It would only be one if we dereferenced now multiple times here which is what I prevented by pulling out the key calculation and save it in t for re-use. Makes sense?

(advance this 0)
cancel-fn))
Copy link
Contributor

Choose a reason for hiding this comment

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

this changes the return value from nil (what advance returns) to a fn.

is that ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not only OK but fixes the mock clock implementation to conform to the expected return value. Quoting from the commit message / PR description:

The latter uncovered an oversight in the mock clock implementation where the in method didn't return a cancellation function, resulting in an NPE. This is now also fixed.

Compare with the real clock implementation and here for where this would fail if we didn't return a function here.

(every [this delay-millis period-millis f]
(assert (p/< 0.0 period-millis))
(let [period (atom period-millis)
Expand Down Expand Up @@ -263,7 +269,9 @@
(catch Throwable e
(manifold.deferred/error! d e)))))
cancel-fn (.in *clock* interval f)]
(manifold.deferred/chain d (fn [_] (cancel-fn)))
(manifold.deferred/on-realized d
(fn [_] (cancel-fn))
(fn [_] (cancel-fn)))
d))

(defn every
Expand Down
18 changes: 17 additions & 1 deletion test/manifold/time_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,23 @@
(testing "delayed function returns failed deferred"
(let [d (d/deferred)]
(d/error! d (Exception. "BOOM"))
(is (thrown? Exception @(t/in 1 (fn [] d)))))))
(is (thrown? Exception @(t/in 1 (fn [] d))))))

(testing "cancelling by completing result deferred"
(let [c (t/mock-clock 0)]
(t/with-clock c
(testing "with success"
(let [n (atom 0)
d (t/in 1 #(swap! n inc))]
(d/success! d :cancel)
(t/advance c 1)
(is (= 0 @n))))
(testing "with error"
(let [n (atom 0)
d (t/in 1 #(swap! n inc))]
(d/error! d (Exception. "cancel"))
(t/advance c 1)
(is (= 0 @n))))))))

(deftest test-every
(let [n (atom 0)
Expand Down