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

Use agent instead of atom to implement mock connection #4

Closed
wants to merge 8 commits into from

Conversation

ChrisBlom
Copy link

Uses an agent to manage the state of the mock connection,
see #2

@vvvvalvalval
Copy link
Owner

Can't merge this right now because it may leave some concurrency issues unaddressed. In particular, the proposed change does not let you read your own writes with (datomic.api/db conn), and I'm not sure that's okay (stay tuned to this StackOverflow question).

Even if it is okay, the need for a proper implementation of sync will probably become more necessary.

I'll keep you posted as I make progress.

@ChrisBlom
Copy link
Author

According to the docs transact returns a completed future, so that would suggest that
after (d/transact conn data) finished, the data is transacted, and your writes should be in (d/db conn) , but it would be good if someone from Datomic can confirm this.

I see now that my implementation is not correct, i've implemened transact-async not transact,
i'll fix this. I this the concurrency issue you mean?

@ChrisBlom
Copy link
Author

I noticed another problem: the future is delivered before the fn that updates the agent returns, so
potentially the agent state is not updated yet once the future is delivered, do you now a clean way to solve this?

@vvvvalvalval
Copy link
Owner

vvvvalvalval commented Dec 7, 2017

I noticed another problem: the future is delivered before the fn that updates the agent returns, so
potentially the agent state is not updated yet once the future is delivered, do you now a clean way to solve this?

@ChrisBlom it is not another problem, it is the same problem :) I had not noticed the issue with transact's implementation.

@vvvvalvalval
Copy link
Owner

@ChrisBlom thanks, I'd appreciate if you could add a little test for the RYW semantics

@vvvvalvalval
Copy link
Owner

And we still need a proper implementation of (d/sync conn t), if you want to give it a shot the best strategy I can think of is adding a watch for to the agent that looks up the basis-t (then self-removes)

(->MockConnection (atom (->MockConnState db [])) (d/next-t db) parent-log (atom nil)))
(let [state-agent (-> (agent (->MockConnState db [] nil))
(add-watch ::force-deliver-promise (fn [_ _ old new]
(when-not (= old new)
Copy link
Author

Choose a reason for hiding this comment

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

should use identical? here

Copy link
Owner

Choose a reason for hiding this comment

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

@ChrisBlom yes, it will be equivalent and faster since the deliver-tx-res field will not be collection-typed.

@ChrisBlom
Copy link
Author

I've implemented (d/sync conn t) and added a test for d/transact-async

@ChrisBlom
Copy link
Author

the implementation is not correct, (d/sync conn t) does not work when the db already has t, please ignore for now

@ChrisBlom ChrisBlom closed this Jan 25, 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
Development

Successfully merging this pull request may close these issues.

2 participants