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

SimpleVCell does not implement Serializable #308

Open
bfabry opened this issue Nov 26, 2015 · 12 comments
Open

SimpleVCell does not implement Serializable #308

bfabry opened this issue Nov 26, 2015 · 12 comments

Comments

@bfabry
Copy link
Contributor

bfabry commented Nov 26, 2015

Clojure functions are normally Serializable if they are AOT'd, however schema attaches some extra information to the class generated for a function when using schema.core/defn, in the form of SimpleVCell, which is a deftype that does not implement java.io.Serializable. As a result functions defined using schema.core/defn can't be serialized. Can be fixed just by adding "Serializable" to the list of interfaces for SimpleVCell.

(deftype SimpleVCell [^:volatile-mutable ^boolean q]
  PSimpleCell
  (get_cell [this] q)
  (set_cell [this x] (set! q x))
  Serializable)
@w01fe
Copy link
Member

w01fe commented Nov 26, 2015

Thanks for the report. This cell is a global singleton used to implement with-fn-validation and such, so it seems like after serializing and deserializing a function it would stop becoming a singleton, and break that functionality (does something similar normally happen with other vars a function refers to?)

On the one hand I don't see the harm in adding the interface, but also don't want to lead people down a path fraught with other issues. Would you mind elaborating a bit on the use case for serializing fns, and what behaviour is typical / expected in situations like this? Thanks!

@bfabry
Copy link
Contributor Author

bfabry commented Nov 26, 2015

In this particular case fn's are being serialized and distributed over a compute cluster by google dataflow. In such a case yeah obviously anything like a mutable global singleton is going to be fairly meaningless, existing concurrently on multiple compute nodes as it does. In that situation I would just expect a function like with-fn-validation to not work, or only work in a certain way :-)

My thinking is functions generated by clojure.core/defn are "serializable (if AOT'd) but buyer beware", so functions generated by prismatic.schema/defn should also be "serializable but buyer beware". I suppose an alternative would be I write my own s/defn that doesn't provide the with-fn-validation functionality (or provides it using a var or something)

@w01fe
Copy link
Member

w01fe commented Nov 28, 2015

Sounds reasonable to me, PR welcome.

FWIW, you could also set compile-fn-validation to false which would have the effect you mention (removes all the validation logic entirely from the macroexpansion)

@bfabry
Copy link
Contributor Author

bfabry commented Nov 28, 2015

Ah but I'm actually more inclined to just have validation on all the time for every function :-) sure I'll try and get a PR up monday.

@bfabry
Copy link
Contributor Author

bfabry commented Dec 15, 2015

hmm, recent addition of (delay for the checker seems to have made this a fair bit harder =/

@w01fe
Copy link
Member

w01fe commented Feb 22, 2016

So the new issue is that Delay isn't serializable? I suppose if functions are, there's no reason why they couldn't be ... although I don't expect that would be easy to change.

Do you see a way to achieve this result without sacrificing performance in the common case?

@bfabry
Copy link
Contributor Author

bfabry commented Feb 22, 2016

A delay isn't serializable because it has state, in general stateful things aren't marked as serializable in clj. For instance a memoized function would not be serializable. I'm not really seeing an easy way to reconcile the performance optimisation and keeping functions serializable, I'm probably going to have to create a fork where all the decisions have to happen at creation time for the fn, ie both whether to compile and whether to validate.

@w01fe
Copy link
Member

w01fe commented Feb 22, 2016

Well, clojure.lang.LazySeq is Serializable, and at its core it seems like just a (transparent) delay of a sequence...

In any case, if you can lay out what you're thinking in terms of a fork, I'd be happy to think about how we might accomplish that within core instead.

@bfabry
Copy link
Contributor Author

bfabry commented Feb 22, 2016

That's true, I guess it's a bit of a violation of that. But I suppose it would be odd if a data structure weren't serializable. shrug

My basic plan is to consolidate the global compile and validate options into one, if it's turned on at the time of fn creation we both compile the validation, and have it turned on. If it's not then we don't. This would mirror how we generally turn on/off validation, which is at an environment level (dev(Y), master(Y), staging(Y), production(N))

@w01fe
Copy link
Member

w01fe commented Feb 22, 2016

So to be clear, is what you want (more or less) is a third option :always for *compile-fn-validation* that compiles every function (that's not :never-validate) to be :always-validate? If so, I think we would accept a patch for something like that unless there are issues I'm not currently seeing.

@bfabry
Copy link
Contributor Author

bfabry commented Feb 23, 2016

that sounds right. I'm going to put something in our backlog to do that, but y'know the real world may intervene :-)

@w01fe
Copy link
Member

w01fe commented Feb 23, 2016

Sounds good, thanks :)

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

No branches or pull requests

2 participants