The order of :clone and :describe init ops is non-determninistc

[vspinu]

(--->
  op  "clone"
  id  "1"
)
(--->
  op  "clone"
  id  "2"
)
(--->
  op  "describe"
  id  "3"
)
(<-
  id  "1"
  new-session  "ddeb097c-5655-4a73-bb2b-f7e7f8963268"
  session  "e155b23a-0a51-4517-9107-3eda47c4e5e4"
  status  ("done")
)
(<-
  id  "3"
  ops  (dict
         apropos  (dict)
         classpath  (dict)
         clone  (dict)
         close  (dict)
         complete  (dict)
         describe  (dict)
         eval  (dict)
         info  (dict)
         inspect-pop  (dict)
         inspect-push  (dict)
         inspect-refresh  (dict)
         inspect-reset  (dict)
         inspect-start  (dict)
         interrupt  (dict)
         load-file  (dict)
         ls-sessions  (dict)
         macroexpand  (dict)
         ns-list  (dict)
         ns-vars  (dict)
         resource  (dict)
         retest  (dict)
         stacktrace  (dict)
         stdin  (dict)
         test  (dict)
         test-stacktrace  (dict)
         toggle-trace  (dict)
         undef  (dict)
       )
  session  "bc70ee1a-4ce8-4ff9-a800-14dc32ce8625"
  status  ("done")
  versions  (dict
              clojure  (dict
                         incremental  1
                         major  1
                         minor  5
                       )
              nrepl  (dict
                       incremental  "3"
                       major  "0"
                       minor  "2"
                       qualifier  ""
                       version-string  "0.2.3"
                     )
            )
)
(--->
  op  "eval"
  session  nil  code  "(try (require 'cider.nrepl)\n                                  (:version-string @(resolve 'cider.nrepl/version))\n                               (catch Throwable _ \"not installed\"))"
  id  "4"
)
(<-
  id  "2"
  new-session  "89ca24cd-849e-41f6-9577-e34ff4dcc7fb"
  session  "70fdefa5-e1fb-4407-846c-8d5244f5ba99"
  status  ("done")
)
(<-
  id  "4"
  session  nil  status  ("unknown-session" "error")
)

Note the error status for request 4. I think the order of the two :clones and the :describe must be enforced. Either by a cascading handlers or by sync requests.

[bbatsov]

Note the error status for request 4. I think the order of the two :clones and the :describe must be enforced. Either by a cascading handlers or by sync requests.

We should aim to reduce the usage of sync requests, so I'd rather have this done by cascading handlers. On a related - ideally we'll do no tooling evals on startup. Solving this might allow us to stuff all the data we need in the describe op's response.

[vspinu]

We should aim to reduce the usage of sync requests.

Startup is a bit special. You really don't want any tool to send requests till the system is up and running. Even if we cascade the CIDER initialization, some guy, some day will add an emacs hookery to send his own requests before the initialization completes.

I think sync requests in this place are not a bad idea. When cascading is used and any of the these requests doesn't complete, the whole CIDER session would behave erratically. With sync requests emacs is stalled and user gets a clear indication of a problem. The code is also considerably simpler.

[bbatsov]

Startup is a bit special. You really don't want any tool to send requests till the system is up and running. Even if we cascade the CIDER initialization, some guy, some day will add an emacs hookery to send his own requests before the initialization completes.

We can't account for bad user decisions.

I think sync requests in this place are not a bad idea. When cascading is used and any of the these requests doesn't complete, the whole CIDER session would behave erratically. With sync requests emacs is stalled and user gets a clear indication of a problem. The code is also considerably simpler.

Every sync request we do is against the nature of nREPL. In essence we're fighting the tooling we have to work with and we shouldn't. The "simplicity" we gain is superficial as we make the code susceptible to lock ups (or temp freezes). The only sensible long term strategy is to aim to reduce the usage of sync requests.

[bbatsov]

To elaborate a bit more - what you say makes perfect sense in principle, but in practice every time I've tried to simplify something in the startup with the use of sync requests things have ended badly for someone. You should recall the most recent problem with the timeouts some people experienced and in the past we had odd problems like some namespaces not being found, total Emacs freezes, etc. Somewhat amusingly none of the problems various people experienced was something I could reproduce. Some much for simplifying things...

I'll think more about the concrete issue at hand.

[vspinu]

every time I've tried to simplify something in the startup with the
use of sync requests things have ended badly for someone. You should
recall the most recent problem with the timeouts some people
experienced and in the past we had odd problems like some namespaces
not being found,

By using async request you are not solving the problem. The timout will
still occur and the impact will be more damaging because the main issue
will not be noticed. Like in the OP - I would not have noticed the
problem if I not have looked at the *nrepl-messages*. Earlier you fall
is the better, and async will help with that.

I naturally agree that more async requests we have is the better, but
some issues do naturally require sync requests - completion, interactive
user feedback, startup are examples.

A lot of stuff has been changed in recent weeks. The request system is
simpler and more robust than it was before. Sync requests are just an
async request under the hood. I believe that in order to overcome the
sync-request "fear" we should give it another try.

[vspinu]

Startup is a bit special. You really don't want any tool to send requests
till the system is up and running. Even if we cascade the CIDER
initialization, some guy, some day will add an emacs hookery to send his own
requests before the initialization completes.

We can't account for bad user decisions.

Then you need to have a flag :init-completed and the user needs to check
that flag in a delayed timer. All internal CIDER functions must also
check that flag before sending requests.

More I think about it more convinced I am that async during the
initialization is a bad idea.

[bbatsov]

By using async request you are not solving the problem. The timout will
still occur and the impact will be more damaging because the main issue
will not be noticed.

Timeout? And I wouldn't call a problem causing the REPL init to fail hard to notice - sure, people have to check the messages log, but it's obvious something is wrong.

I believe that in order to overcome the
sync-request "fear" we should give it another try.

I don't "fear" sync-requests; I'm just fairly certain that the most obvious solutions are rarely the best solutions. A lot of quick fixes brought the code to its sorry current state and I'm still paying the price for those... Sometimes the problem you really have to solve is not the one that's the most apparent one.

Regardless of recent improvements they still fall short in some aspects:

• issue reporting there sucks; obviously we need some middleware convention to pass the exception info, but right now non-eval sync failures are totally confusing for users
• the timeout thing is a hack - ideally we'll have some way to understand something went wrong with a request

[vspinu]

people have to check the messages log, but it's obvious something is
wrong.

Well, common! *nrepl-messages* is not even there by default. In cider
the difference between "something is wrong" and "initialization timed
out" can be a matter of a week of debugging as #671 illustrated.

I'm just fairly certain that the most obvious solutions are rarely
the best solutions.

As you said many times before - the best solution is often the simplest.

A lot of quick fixes brought the code to its sorry current state and
I'm still paying the price for those... Sometimes the problem you
really have to solve is not the one that's the most apparent one.

I agree but let's not get drifted into general discussion on
programming. The current problem is apparent and the sync solution is
not a "quick fix", but rather a necessity. No-one should be able to use
the system till it's fully initialized.

Regardless of recent improvements they still fall short in some aspects:

• issue reporting there sucks; obviously we need some middleware convention to
  pass the exception info, but right now non-eval sync failures are totally
  confusing for users
• the timeout thing is a hack - ideally we'll have some way to understand
  something went wrong with a request

Sure, but this is somewhat irrelevant to the current problem. The :clone
and :describe handlers are treated specially and are very simple. Having
those in sync or async requests hardly makes any difference.

I am making a new PR. It makes code simpler and easier to reason
about. Please have a fresh look tomorrow.

[bbatsov]

I agree but let's not get drifted into general discussion on
programming. The current problem is apparent and the sync solution is
not a "quick fix", but rather a necessity. No-one should be able to use
the system till it's fully initialized.

Fair enough.