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

New task: socket-server #549

Merged
merged 1 commit into from
Mar 3, 2017

Conversation

pesterhazy
Copy link
Contributor

@pesterhazy pesterhazy commented Dec 23, 2016

Exposes the Socket Server functionality available in Clojure 1.8 or above as a new task. By default, the handler function is clojure.core.server/repl, but a different handler can be specified.

This is useful because Clojure's standard way of enabling the socket server, by setting a system property, doesn't work well with boot (#453).

Wheres boot repl --server, provides nREPL functionality used in CIDER, boot socket-server can be used e.g. with Emacs's inf-clojure mode. It starts up much faster than boot repl --server (12s vs 80s), and is more easily accessible using nc or telnet. But compared to REPLy it is pretty bare-bones and doesn't provide advanced features like auto-completion or beautiful stack traces.

This commit adds a new task rather than extending repl. The reason is that Socket Servers provides not just REPL but server functionality in general, and that repl semantics are already confusing because of how --server and --client flags work.

Like repl, socket-server defaults to picking a port at random and stores it in a file, .socket-port, in the current directory.

Fixes #453

Copy link
Member

@martinklepsch martinklepsch left a comment

Choose a reason for hiding this comment

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

Great addition IMO! 👍 Just a few minor suggestions.

a accept ACCEPT sym "Namespaced symbol of the accept function to invoke."]
(let [repl-soc (delay (core/launch-socket-server *opts*))]
(core/with-pass-thru [fs]
@repl-soc)))
Copy link
Member

Choose a reason for hiding this comment

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

This starts the server but doesn't shut it down afterwards. Not sure if that's necessary, maybe you can comment on that. See boot.core/cleanup and clojure.core.server/stop-server if you decide to add it.

https://github.com/boot-clj/boot/blob/master/doc/boot.core.md#cleanup

Copy link
Contributor

@Deraen Deraen Jan 5, 2017

Choose a reason for hiding this comment

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

Cleanup task is currently problematic with REPL. If boot is run inside repl task, cleanup from that boot-in-boot would kill also host REPL. So, for now, this probably shouldn't use cleanup: #554

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know much about boot-in-boot so I'll defer to your judgment that cleaning up is dangerous right now.

(let [effective-port (-> ((resolve 'clojure.core.server/start-server) opts)
(.getLocalPort))]
(doto (io/file ".socket-port") .deleteOnExit (spit effective-port))
(util/info "Socket server started on port %s on host %s.\n" effective-port (:host opts)))))
Copy link
Member

Choose a reason for hiding this comment

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

I think this function should be moved into a namespace that is available within pods.

And could there be a more useful return value than nil? Maybe the name (which is needed when stopping servers) or a function that stops the server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR to return the name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure which namespace would appropriate. I also don't understand the "accessible from pods" distinction - could you elaborate why that would be useful?

Copy link
Member

Choose a reason for hiding this comment

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

Basically boot.core cannot be loaded in a pod because it has stateful stuff that we don't want to replicate. There may be a moment where you want to launch a socket server in a pod though which, with the function in boot.core, would require copying the implementation somewhere where it can be loaded by the pod.

For this reason I'm suggesting we put the function somewhere where it is also available in pods. That could be boot.util or boot.pod, probably there are more, but these two come to mind...

@pesterhazy
Copy link
Contributor Author

pesterhazy commented Jan 23, 2017

Looking into this again, I thought about your suggestion, @martinklepsch. boot.pod or boot.util don't seem appropriate places to put this code as they contain fns to do with pods or utilities.

I don't fully understand why code in a pod would want to run launch-socket-server as opposed to just calling clojure.core.server/start-server. In my view launch-socket-server is just the implementation of the deftask.

I chose core.clj so the fn sits next to launch-nrepl. Is part of the concern that we shouldn't clutter core.clj? In that case maybe boot.task.built-in would be better?

@martinklepsch
Copy link
Member

martinklepsch commented Jan 23, 2017

launch-nrepl runs an nrepl server in a pod via boot.repl/launch-nrepl 🙂

The concern is not cluttering core but instead making it available as a general utility (which includes usage from within pods) which is not possible when it's put into core.

Seeing that there is boot.repl maybe it should just go there?

Edit: if you look closely at boot.core/launch-nrepl you see it's mostly a shim around boot.repl/launch-nrepl that handles some defaults around pods and allows passing the name of a pod where the nrepl server should be launched.

@pesterhazy
Copy link
Contributor Author

Would this mean that the code needs to be run in a pod, like the nrepl server?

@martinklepsch
Copy link
Member

@pesterhazy no, you can still run it in your regular REPL just like that or wherever you want of course (theoretically everything is in a pod but that's more of a detail and probably not what you're concerned about).

Exposes the Socket Server functionality available in Clojure 1.8 or
above as a task. By default, the handler function is
clojure.core.server/repl, but a different handler can be specified.
@pesterhazy
Copy link
Contributor Author

pesterhazy commented Feb 6, 2017

I've updated the PR as per the comments above

@martinklepsch
Copy link
Member

I think this is good to merge but maybe someone else can take a look too? @Deraen @alandipert?

@alandipert
Copy link
Contributor

Just took a look. Awesome, thanks!

Looking at this I thought it might be cool if launch-socket-server took an optional pod name argument. Then we'd have a way to get simultaneous pod REPLs. If it took the argument, the pod name should perhaps influence the name of .socket-port, or maybe not create such a file at all.

@alandipert alandipert merged commit aea894d into boot-clj:master Mar 3, 2017
@danielsz
Copy link
Contributor

danielsz commented Mar 3, 2017

Great work, folks. Thank you!

@pesterhazy
Copy link
Contributor Author

@alandipert, thanks for merging!

Could you outline a scenario where opening a socket server into a pod would be useful?

@martinklepsch
Copy link
Member

@pesterhazy I think what @alandipert is thinking of are situations where you want to "look into a pod" but don't have a reference to the pod object and instead only have the name.

I have never needed to look into a pod like that myself though.

@alandipert
Copy link
Contributor

@pesterhazy getting a REPL in a pod with the repl task's -P option is useful for developing Boot, and for developing and debugging tasks that use pods.

Using repl -P to get a REPL inside a pod is especially useful when tasks don't make their pods public. It's a way to run code in them anyway 😎

@pesterhazy pesterhazy mentioned this pull request Jun 16, 2017
@martinklepsch martinklepsch mentioned this pull request Jun 16, 2017
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.

5 participants