defservice can refer to stale protocols

[vemv]

Describe the Bug

I use TK in a REPL-heavy manner, using (reset) repeatedly and also running tests from the REPL or CIDER.

Sometimes, quite rarely I can hit an error in the form:

java.lang.IllegalArgumentException: No implementation of method: :lookup of protocol: #'my.ns/FooService found for class: my.ns$reify__695487

Where the former is a defprotocol and the latter is a defservice. They live in the same ns, one after the other respectively.

(Original namespaces are redacted)

Environment

TK 3.1.0

Additional Context

This error is analog to https://github.com/clojure/tools.namespace/tree/06de425a09333456319d9d06e96c181e4c2d7c0a#warnings-for-protocols , with the difference that a vanilla defprotocol+defrecord case can be worked around (by re- evaluating the defrecord isolatedly in the repl), while I wasn't able to work around defprotocol+defservice.

Maybe there's some caching going on?

[vemv]

IME (I have worked a lot in this area in the last couple years) reify and defrecord are both subject to this kind of errors, especially if using tools.namespace.

The following code (called by defservice) uses reify twice:

trapperkeeper/src/puppetlabs/trapperkeeper/services.clj

Lines 85 to 94 in c943510

	`(reify ServiceDefinition
	(service-def-id [this] ~service-id)
	;; service map for prismatic graph
	(service-map [this]
	{~service-id
	;; the main service fnk for the app graph. we add metadata to the fnk
	;; arguments list to specify an explicit output schema for the fnk
	(fnk service-fnk# :- ~output-schema
	~(conj dependencies 'tk-app-context 'tk-service-refs)
	(let [svc# (reify

If metadata-based protocol implementation was used instead, one would get rid of the error.

Maybe you could add :extend-via-metadata to the 3 protocols in https://github.com/puppetlabs/trapperkeeper/blob/c943510c08acde6b1e3c2b55b450fef325734022/src/puppetlabs/trapperkeeper/services.clj and offer an alternative version of defservice which ditches reify?

(will give it a shot meanwhile)

[frenchy64]

I took another look at this issue. Perhaps it has something to do with the dereferenced protocol var when we build the service graph instead of a var deref on this line.

We noticed our code was more reliable using get-service rather than service graph functions, perhaps this is the reason.

[frenchy64]

Might be caused by https://clojure.atlassian.net/browse/CLJ-1796, though I can't think of a concrete reproduction. It has similar symptoms. Please vote on the linked ask.clojure.org answer to get it some attention.

[vemv]

Simply for completeness relative to the issue I opened, https://github.com/threatgrid/trapperkeeper/tree/extend-via-metadata-2 worked really well for me

[frenchy64]

Yes, on further thought, it could not have been caused by https://clojure.atlassian.net/browse/CLJ-1796 because TK always implements interfaces directly via reify, and that issue is about extend. The :extend-via-metadata solution I think is the last lead to a diagnosis (still have no idea though!).

[vemv]

Isn't an accurate diagnostic that simply protocols / reifies get reloaded in the wrong order? As one (refresh)es or re-evals things by hand.

i.e. with code reloading, one gets a new Java interface defined for a given protocol, and then a reify that implements the old interface for that protocol.

It's basically what is mentioned in https://github.com/clojure/tools.namespace/tree/06de425a09333456319d9d06e96c181e4c2d7c0a#warnings-for-protocols - I would be somewhat surprised if TK's issue was fundamentally different from that one.

TK might be making this even more likely to happen given it has certain affordances that make dependency trees less explicit (namely: fostering fn injection over vanilla defprotocols), so tools.namespace's dep graph won't be as accurate.