Clojure

Clojure socket server

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.7
  • Fix Version/s: Release 1.8
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

Programs often want to provide REPLs to users in contexts when a) network communication is desired, b) capturing stdio is difficult, or c) when more than one REPL session is desired. In addition, tools that want to support REPLs and simultaneous conversations with the host are difficult with a single stdio REPL as currently provided by Clojure.

Tooling and users often need to enable a REPL on a program without changing the program, e.g. without asking author or program to include code to start a REPL host of some sort. Thus a solution must be externally and declaratively configured (no user code changes). A REPL is just a special case of a socket service. Rather than provide a socket server REPL, provide a built-in socket server that composes with the existing repl function.

For design background, see: http://dev.clojure.org/display/design/Socket+Server+REPL

Start a socket server by supplying an extra system property (classpath and clojure.main here, but this would generally be starting your own app instead - we won't use the repl it starts):

java -cp target/classes -Dclojure.server.repl="{:port 5555 :accept clojure.core.server/repl}" clojure.main

where options are:

  • address = host or address, defaults to loopback
  • port = port, required
  • accept = namespaced function to invoke on socket accept, required
  • args = sequential collection of args to pass to accept
  • bind-err = defaults to true, binds err to out stream
  • server-daemon = defaults to true, socket server thread doesn't block exit
  • client-daemon = defaults to true, socket client threads don't block exit

Run a repl client using telnet:

$ telnet 127.0.0.1 5555
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
user=> (println "hello")
hello
nil
user=> clojure.core.server/*session*
{:server "repl", :client "1"}
user=> (ns foo)
nil
foo=> (+ 1 1)
2
foo=>

Patch: clj-1671-13.patch

  1. clj-1671-10.patch
    25/Aug/15 9:51 AM
    14 kB
    Alex Miller
  2. clj-1671-11.patch
    25/Aug/15 3:37 PM
    14 kB
    Alex Miller
  3. clj-1671-12.patch
    04/Sep/15 4:42 PM
    13 kB
    Alex Miller
  4. clj-1671-13.patch
    09/Sep/15 9:26 AM
    9 kB
    Alex Miller
  5. clj-1671-2.patch
    11/Mar/15 11:31 AM
    5 kB
    Alex Miller
  6. clj-1671-3.patch
    12/Mar/15 12:06 PM
    11 kB
    Alex Miller
  7. clj-1671-4.patch
    13/Mar/15 11:29 AM
    13 kB
    Alex Miller
  8. clj-1671-5.patch
    02/Jul/15 1:39 PM
    8 kB
    Alex Miller
  9. clj-1671-6.patch
    08/Jul/15 9:17 AM
    8 kB
    Alex Miller
  10. clj-1671-7.patch
    14/Aug/15 1:15 AM
    10 kB
    Alex Miller
  11. clj-1671-8.patch
    19/Aug/15 3:27 PM
    12 kB
    Alex Miller
  12. clj-1671-9.patch
    25/Aug/15 9:14 AM
    14 kB
    Alex Miller

Activity

Hide
Timothy Baldridge added a comment -

Could we perhaps keep this as a contrib library? This ticket simply states "The goal is to provide a simple streaming socket repl as part of Clojure." What is the rationale for the "part of Clojure" bit?

Show
Timothy Baldridge added a comment - Could we perhaps keep this as a contrib library? This ticket simply states "The goal is to provide a simple streaming socket repl as part of Clojure." What is the rationale for the "part of Clojure" bit?
Hide
Alex Miller added a comment -

We want this to be available as a Clojure.main option. It's all additive - why wouldn't you want it in the box?

Show
Alex Miller added a comment - We want this to be available as a Clojure.main option. It's all additive - why wouldn't you want it in the box?
Hide
Timothy Baldridge added a comment -

It never has really been too clear to me why some features are included in core, while others are kept in contrib. I understand that some are simply for historical reasons, but aside from that there doesn't seem to be too much of a philosophy behind it.

However it should be noted that since patches to clojure are much more guarded it's sometimes nice to have certain features in contrib, that way they can evolve with more rapidity than the one release a year that clojure has been going through.

But aside from those issues, I've found that breaking functionality into modules forces the core of a system to become more configurable. Perhaps I would like to use this repl socket feature, but pipe the data over a different communication protocol, or through a different serializer. If this feature were to be coded as a contrib library it would expose extension points that others could use to add additional functionality.

So I guess, all that to say, I'd prefer a tool I can compose rather than a pre-built solution.

Show
Timothy Baldridge added a comment - It never has really been too clear to me why some features are included in core, while others are kept in contrib. I understand that some are simply for historical reasons, but aside from that there doesn't seem to be too much of a philosophy behind it. However it should be noted that since patches to clojure are much more guarded it's sometimes nice to have certain features in contrib, that way they can evolve with more rapidity than the one release a year that clojure has been going through. But aside from those issues, I've found that breaking functionality into modules forces the core of a system to become more configurable. Perhaps I would like to use this repl socket feature, but pipe the data over a different communication protocol, or through a different serializer. If this feature were to be coded as a contrib library it would expose extension points that others could use to add additional functionality. So I guess, all that to say, I'd prefer a tool I can compose rather than a pre-built solution.
Hide
Rich Hickey added a comment -

Please move discussions on the merits of the idea to the dev list. Comments should be about the work of resolving the ticket, approach taken by the patch, quality/perf issues etc.

Show
Rich Hickey added a comment - Please move discussions on the merits of the idea to the dev list. Comments should be about the work of resolving the ticket, approach taken by the patch, quality/perf issues etc.
Hide
Colin Jones added a comment -

I see that context (a) of the rationale is that network communication is desired, which sounds to me like users of this feature may want to communicate across hosts (whether in VMs or otherwise). Is that the case?

If so, it seems like specifying the address to bind to (e.g. "0.0.0.0", "::", "127.0.0.1", etc.) may become important as well as the existing port option. This way, someone who wants to communicate across hosts (or conversely, lock down access to local-only) can make that decision.

Show
Colin Jones added a comment - I see that context (a) of the rationale is that network communication is desired, which sounds to me like users of this feature may want to communicate across hosts (whether in VMs or otherwise). Is that the case? If so, it seems like specifying the address to bind to (e.g. "0.0.0.0", "::", "127.0.0.1", etc.) may become important as well as the existing port option. This way, someone who wants to communicate across hosts (or conversely, lock down access to local-only) can make that decision.
Hide
Alex Miller added a comment -

Colin - agreed. There are many ways to potentially customize what's in there so we need to figure out what's worth doing, both in the function and via the command line.

I think address is clearly worth having via the function and possibly in the command line too.

Show
Alex Miller added a comment - Colin - agreed. There are many ways to potentially customize what's in there so we need to figure out what's worth doing, both in the function and via the command line. I think address is clearly worth having via the function and possibly in the command line too.
Hide
Kevin Downey added a comment -

I find the exception printing behavior really odd. for a machine you want an exception as data, but you also want some indication of if the data is an error or not, for a human you wanted a pretty printed stacktrace. making the socket repl default to printing errors this way seems to optimize for neither.

Show
Kevin Downey added a comment - I find the exception printing behavior really odd. for a machine you want an exception as data, but you also want some indication of if the data is an error or not, for a human you wanted a pretty printed stacktrace. making the socket repl default to printing errors this way seems to optimize for neither.
Hide
Rich Hickey added a comment -

Did you miss the #error tag? That indicates the data is an error. It is likely we will pprint the error data, making it not bad for both purposes

Show
Rich Hickey added a comment - Did you miss the #error tag? That indicates the data is an error. It is likely we will pprint the error data, making it not bad for both purposes
Hide
Alex Miller added a comment -

New -4 patch changes:

  • clojure.core/throwable-as-map now public and named clojure.core/Throwable->map
  • catch and ignore SocketException without printing in socket server repl (for client disconnect)
  • functions to print as message and as data are now: clojure.main/err-print and clojure.main/err->map. All defaults and docs updated.
Show
Alex Miller added a comment - New -4 patch changes:
  • clojure.core/throwable-as-map now public and named clojure.core/Throwable->map
  • catch and ignore SocketException without printing in socket server repl (for client disconnect)
  • functions to print as message and as data are now: clojure.main/err-print and clojure.main/err->map. All defaults and docs updated.
Hide
David Nolen added a comment - - edited

Is there any reason to not allow supplying :eval in addition to :use-prompt? In the case of projects like ClojureCLR + Unity eval generally must happen on the main thread. With :eval as something which can be configured, REPL sessions can queue forms to be eval'ed with the needed context (current ns etc.) to the main thread.

Show
David Nolen added a comment - - edited Is there any reason to not allow supplying :eval in addition to :use-prompt? In the case of projects like ClojureCLR + Unity eval generally must happen on the main thread. With :eval as something which can be configured, REPL sessions can queue forms to be eval'ed with the needed context (current ns etc.) to the main thread.
Hide
Kevin Downey added a comment -

I did see the #error tag, but throwables print with that tag regardless of if they are actually thrown or if they are just the value returned from a function. Admittedly returning exceptions as values is not something generally done, but the jvm does distinguish between a return value and a thrown exception. Having a repl that doesn't distinguish between the two strikes me as an odd design. The repl you get from clojure.main currently prints the message from a thrown uncaught throwable, and on master prints with #error if you have a throwable value, so it distinguishes between an uncaught thrown throwable and a throwable value. That obviously isn't great for tooling because you don't get a good data representation in the uncaught case.

It looks like the most recent patch does pretty print uncaught throwables, which is helpful for humans to distinguish between a returned value and an uncaught throwable.

Show
Kevin Downey added a comment - I did see the #error tag, but throwables print with that tag regardless of if they are actually thrown or if they are just the value returned from a function. Admittedly returning exceptions as values is not something generally done, but the jvm does distinguish between a return value and a thrown exception. Having a repl that doesn't distinguish between the two strikes me as an odd design. The repl you get from clojure.main currently prints the message from a thrown uncaught throwable, and on master prints with #error if you have a throwable value, so it distinguishes between an uncaught thrown throwable and a throwable value. That obviously isn't great for tooling because you don't get a good data representation in the uncaught case. It looks like the most recent patch does pretty print uncaught throwables, which is helpful for humans to distinguish between a returned value and an uncaught throwable.
Hide
Kevin Downey added a comment -

alex: saying this is all additive, when it has driven changes to how things are printed, using the global print-method, rings false to me

Show
Kevin Downey added a comment - alex: saying this is all additive, when it has driven changes to how things are printed, using the global print-method, rings false to me
Hide
Sam Ritchie added a comment -

This seems like a pretty big last minute addition for 1.7. What's the rationale for adding it here vs deferring to 1.8, or trying it out as a contrib first?

Show
Sam Ritchie added a comment - This seems like a pretty big last minute addition for 1.7. What's the rationale for adding it here vs deferring to 1.8, or trying it out as a contrib first?
Hide
Alex Miller added a comment -

Kevin: changing the fallthrough printing for things that are unreadable to be readable should be useful regardless of the socket repl. It shouldn't be a change for existing programs (unless they're relying on the toString of objects without print formats).

Sam: Rich wants it in the box as a substrate for tools.

Show
Alex Miller added a comment - Kevin: changing the fallthrough printing for things that are unreadable to be readable should be useful regardless of the socket repl. It shouldn't be a change for existing programs (unless they're relying on the toString of objects without print formats). Sam: Rich wants it in the box as a substrate for tools.
Hide
Alex Miller added a comment -

Marking incomplete, pending at least the repl exit question.

Show
Alex Miller added a comment - Marking incomplete, pending at least the repl exit question.
Hide
Laurent Petit added a comment -

Hello, I intend to work on this, if it appears it still has a good probability of being included in clojure 1.7.
There hasn't been much visible activity on it lately.
What is the current status of the pending question, and do you think it will still make it in 1.7?

Show
Laurent Petit added a comment - Hello, I intend to work on this, if it appears it still has a good probability of being included in clojure 1.7. There hasn't been much visible activity on it lately. What is the current status of the pending question, and do you think it will still make it in 1.7?
Hide
Alex Miller added a comment -

This has been pushed to 1.8 and is on my plate. The direction has diverged quite a bit from the original description and we don't expect to modify clojure.main as is done in the prior patches. So, I would recommend not working on it as described here.

Show
Alex Miller added a comment - This has been pushed to 1.8 and is on my plate. The direction has diverged quite a bit from the original description and we don't expect to modify clojure.main as is done in the prior patches. So, I would recommend not working on it as described here.
Hide
Laurent Petit added a comment -

OK thanks for the update.

Is the discussion about the new design / goal (you say the direction has diverged) available somewhere so that I can keep in touch with what the Hammock Time is producing? Because on my own hammock time I'm doing some mental projections for CCW support of this, based on what is publicly available here -

Also, as soon as you have something available for testing please don't hesitate to ping me, I'll see what I can do to help depending on my schedule. Cheers.

Show
Laurent Petit added a comment - OK thanks for the update. Is the discussion about the new design / goal (you say the direction has diverged) available somewhere so that I can keep in touch with what the Hammock Time is producing? Because on my own hammock time I'm doing some mental projections for CCW support of this, based on what is publicly available here - Also, as soon as you have something available for testing please don't hesitate to ping me, I'll see what I can do to help depending on my schedule. Cheers.
Hide
Alex Miller added a comment -
Show
Alex Miller added a comment - Some design work is here - http://dev.clojure.org/display/design/Socket+Server+REPL.
Hide
Laurent Petit added a comment -

Thanks for the link. It seems that the design is totally revamped indeed. Better to wait then.

Show
Laurent Petit added a comment - Thanks for the link. It seems that the design is totally revamped indeed. Better to wait then.
Hide
Andy Fingerhut added a comment -

Alex, just a note that the Java method getLoopbackAddress [1] appears to have been added with Java 1.7, so your patches that use that method do not compile with Java 1.6. If the plan was for the next release of Clojure to drop support for Java 1.6 anyway, then no worries.

[1] http://docs.oracle.com/javase/7/docs/api/java/net/InetAddress.html#getLoopbackAddress%28%29

Show
Andy Fingerhut added a comment - Alex, just a note that the Java method getLoopbackAddress [1] appears to have been added with Java 1.7, so your patches that use that method do not compile with Java 1.6. If the plan was for the next release of Clojure to drop support for Java 1.6 anyway, then no worries. [1] http://docs.oracle.com/javase/7/docs/api/java/net/InetAddress.html#getLoopbackAddress%28%29
Hide
Stuart Halloway added a comment - - edited

Lifecycle concerns

1. atoms are weak when actions (starting threads / sockets) are coordinated with recorded state (the map)
2. I see why the plumbing needs access to socket, but what is the motivation for expoosing it to outside code, seems like opportunity to break stuff
3. stating a server has a race condition
4. what happens if somebody wants to call start-server explicitly – how do they know whether that happens before or after config-driven process launches
5. what guarantees about when config-driven launch happens, vis-a-vis other startup-y things
6. is there a use for stop-servers other than shutdown?
7. does Clojure now need a shutdown-clojure-resources? I don't want to have to remember shutdown-agents, plus stop-servers, plus whatever we add next year
8. what happens on misconfiguration (e.g. nonexistent namespace)? will other servers still launch? what thread dies, and where does is report? will the app main process die before even reaching the user?

Show
Stuart Halloway added a comment - - edited Lifecycle concerns 1. atoms are weak when actions (starting threads / sockets) are coordinated with recorded state (the map) 2. I see why the plumbing needs access to socket, but what is the motivation for expoosing it to outside code, seems like opportunity to break stuff 3. stating a server has a race condition 4. what happens if somebody wants to call start-server explicitly – how do they know whether that happens before or after config-driven process launches 5. what guarantees about when config-driven launch happens, vis-a-vis other startup-y things 6. is there a use for stop-servers other than shutdown? 7. does Clojure now need a shutdown-clojure-resources? I don't want to have to remember shutdown-agents, plus stop-servers, plus whatever we add next year 8. what happens on misconfiguration (e.g. nonexistent namespace)? will other servers still launch? what thread dies, and where does is report? will the app main process die before even reaching the user?
Hide
Alex Miller added a comment -

1. it's not in the patch, but the intention is that in the runtime on startup there is a call to (start-servers (System/getProperties)) and generally you're not starting servers on the fly (although it's broken out to make that possible).
2. just trying to make resources available, not sure how much locking down we want/need
3. I'm expecting this to be a startup thing primarily
4. I'm assuming the config-driven process will start in the runtime and thus it will happen first. Not sure how much we need to support the manual stuff - it just seemed like a good idea to make it possible.
5. dunno, haven't looked at where to do that yet. Probably somewhere similar to data_readers stuff?
6. no
7. the threads are daemons by default meaning that it will shut down regardless. If you set the daemon properties to false, then you're in control and need to call stop-servers where it's appropriate.
8. I thought about these questions and do not have good answers. Lots more of that stuff needs to be handled.

Show
Alex Miller added a comment - 1. it's not in the patch, but the intention is that in the runtime on startup there is a call to (start-servers (System/getProperties)) and generally you're not starting servers on the fly (although it's broken out to make that possible). 2. just trying to make resources available, not sure how much locking down we want/need 3. I'm expecting this to be a startup thing primarily 4. I'm assuming the config-driven process will start in the runtime and thus it will happen first. Not sure how much we need to support the manual stuff - it just seemed like a good idea to make it possible. 5. dunno, haven't looked at where to do that yet. Probably somewhere similar to data_readers stuff? 6. no 7. the threads are daemons by default meaning that it will shut down regardless. If you set the daemon properties to false, then you're in control and need to call stop-servers where it's appropriate. 8. I thought about these questions and do not have good answers. Lots more of that stuff needs to be handled.
Hide
Alex Miller added a comment -

More feedback forwarded from Stu:

1. Need to switch from atom to locking around the server/client state map.
2. It is not clear to me exactly what gets pushed by :repl/push – what does "like ns" mean?
3. :repl/quit feels categorically different from push and pull. People may come to expect this on all Clojure REPLs, should we install it at the bottom instead of in the REPL server?
4. The design anticipates the possibility of using multiple connections sharing state to implement a higher level "session". I think we should implement an example of this, both to flesh out the idea and as an example for users.
5. I am opposed to adding variety to the config options more work for unclear payoff and would prefer to leave the file-based option and REPL convenience as a possibility for later
6. We have one name too few - it doesn't make sense for client id to be bound to a tuple that includes server-name and client id the word 'coordinates' has been used in some places for the bigger thing
7. Explicitly enumerate where exceptions end up may be sufficient to document that UncaughtExceptionHandler gets all of them
8. Should programs have direct access to sockets? pro: more control (e.g. design says you can grab socket from server state map and close it). con: leaking abstraction

Show
Alex Miller added a comment - More feedback forwarded from Stu: 1. Need to switch from atom to locking around the server/client state map. 2. It is not clear to me exactly what gets pushed by :repl/push – what does "like ns" mean? 3. :repl/quit feels categorically different from push and pull. People may come to expect this on all Clojure REPLs, should we install it at the bottom instead of in the REPL server? 4. The design anticipates the possibility of using multiple connections sharing state to implement a higher level "session". I think we should implement an example of this, both to flesh out the idea and as an example for users. 5. I am opposed to adding variety to the config options more work for unclear payoff and would prefer to leave the file-based option and REPL convenience as a possibility for later 6. We have one name too few - it doesn't make sense for client id to be bound to a tuple that includes server-name and client id the word 'coordinates' has been used in some places for the bigger thing 7. Explicitly enumerate where exceptions end up may be sufficient to document that UncaughtExceptionHandler gets all of them 8. Should programs have direct access to sockets? pro: more control (e.g. design says you can grab socket from server state map and close it). con: leaking abstraction
Hide
Alex Miller added a comment -

1. Updating patch (but geez does this feel weird to do in Clojure)
2. I'm not entirely sure what info a tooling connection needs about a user's state. *ns* is an obvious one. Maybe *e or the other repl vars too? In some ways this is a similar problem to binding conveyance - giving one thread another's dynamic "state".
3. Agreed on :repl/quit. I had a version of this patch that actually added the "command" capability to the base repl and installed :repl/quit as a default command. I think that's a better answer but I did it this way to make the patch additive and easier to evaluate, idea-wise. So, in other words, agreed and that's do-able. Repl invokers would then need a way to install new commands, which the socket repl could do.
4. I think the "session" notion is a more useful way of saying this than I have done so far. With the benefit of time, I'm not happy with the push/pull stuff and I don't think it really solves the problem. In particular, it would be better if no explicit actions were required for the "push" part. I will do some more design thinking on this on the design page.
5. ok
6. Again, bound up in the push/pull stuff and maybe not necessary.
7. Will add to design page.
8. Given the locking and other stuff, I think I'd now say no - there should be an API for closing etc but not direct access to these.

Show
Alex Miller added a comment - 1. Updating patch (but geez does this feel weird to do in Clojure) 2. I'm not entirely sure what info a tooling connection needs about a user's state. *ns* is an obvious one. Maybe *e or the other repl vars too? In some ways this is a similar problem to binding conveyance - giving one thread another's dynamic "state". 3. Agreed on :repl/quit. I had a version of this patch that actually added the "command" capability to the base repl and installed :repl/quit as a default command. I think that's a better answer but I did it this way to make the patch additive and easier to evaluate, idea-wise. So, in other words, agreed and that's do-able. Repl invokers would then need a way to install new commands, which the socket repl could do. 4. I think the "session" notion is a more useful way of saying this than I have done so far. With the benefit of time, I'm not happy with the push/pull stuff and I don't think it really solves the problem. In particular, it would be better if no explicit actions were required for the "push" part. I will do some more design thinking on this on the design page. 5. ok 6. Again, bound up in the push/pull stuff and maybe not necessary. 7. Will add to design page. 8. Given the locking and other stuff, I think I'd now say no - there should be an API for closing etc but not direct access to these.
Hide
Laurent Petit added a comment - - edited

Just to check, I'd like to understand if we share the same detail view of how the `:accept` key will work.

Its default value is `clojure.repl/repl`.

I expect to be able to specify a value like `my.not.yet.loaded.namespace/my-repl-fn` and that the server code will try to require `my.not.yet.loaded.namespace` from the java classpath.

Also, when will the resolution of `:accept` take place? Before or after all the -e and -i expressions / files have been evaluated? That is, will it be possible to load `my.not.yet.loaded.namespace` via a -i option or in the list of files to load, instead of having to tweak the classpath? (Some tools / IDEs abstracting away the notion of classpath, program arguments, jvm parameters make it difficult if possible at all to tweak the classpath, but really easy to add JVM parameters / program arguments before launching.

Show
Laurent Petit added a comment - - edited Just to check, I'd like to understand if we share the same detail view of how the `:accept` key will work. Its default value is `clojure.repl/repl`. I expect to be able to specify a value like `my.not.yet.loaded.namespace/my-repl-fn` and that the server code will try to require `my.not.yet.loaded.namespace` from the java classpath. Also, when will the resolution of `:accept` take place? Before or after all the -e and -i expressions / files have been evaluated? That is, will it be possible to load `my.not.yet.loaded.namespace` via a -i option or in the list of files to load, instead of having to tweak the classpath? (Some tools / IDEs abstracting away the notion of classpath, program arguments, jvm parameters make it difficult if possible at all to tweak the classpath, but really easy to add JVM parameters / program arguments before launching.
Hide
Alex Miller added a comment -

Hey Laurent,

Keep in mind this is a work in progress still. The idea here is that you can pass these system properties when running ANY Clojure program. The Clojure runtime will scan system properties and start socket servers as needed (that hook is not yet installed).

So you can do clojure.main if you like but if you're passing the server system properties the socket server will be launched. I have not determined yet exactly where that RT loading will happen but I would expect it to be prior to the -i or -e being evaluated.

Show
Alex Miller added a comment - Hey Laurent, Keep in mind this is a work in progress still. The idea here is that you can pass these system properties when running ANY Clojure program. The Clojure runtime will scan system properties and start socket servers as needed (that hook is not yet installed). So you can do clojure.main if you like but if you're passing the server system properties the socket server will be launched. I have not determined yet exactly where that RT loading will happen but I would expect it to be prior to the -i or -e being evaluated.
Hide
Laurent Petit added a comment -

Sure, I know this is WIP, and that's why I find it valuable to add my 2 cents now, rather that when things are almost wrapped up.

So it will be triggered not by the fact that clojure.main is invoked, but during the initialization of the clojure runtime ?

Then it seems indeed difficult to ask to wait for clojure.main to have finished starting.

My first remark still stands, though:

  • the fully qualified var symbol String passed as value of `:accept` parameter should not require that its namespace be already loaded, or this will really limit to choice of vars that can be passed on. Some dynamic `require` call and var `resolve` should be taking place here.
Show
Laurent Petit added a comment - Sure, I know this is WIP, and that's why I find it valuable to add my 2 cents now, rather that when things are almost wrapped up. So it will be triggered not by the fact that clojure.main is invoked, but during the initialization of the clojure runtime ? Then it seems indeed difficult to ask to wait for clojure.main to have finished starting. My first remark still stands, though:
  • the fully qualified var symbol String passed as value of `:accept` parameter should not require that its namespace be already loaded, or this will really limit to choice of vars that can be passed on. Some dynamic `require` call and var `resolve` should be taking place here.
Hide
Alex Miller added a comment -

Oh sorry, I thought you were just confirming what was already there - the current patch does execute a require for the namespace containing the accept function.

Show
Alex Miller added a comment - Oh sorry, I thought you were just confirming what was already there - the current patch does execute a require for the namespace containing the accept function.
Hide
Alex Miller added a comment -

The -8 patch integrates the server startup into the Clojure runtime. It also improves validation and error-handling on the :repl commands and attached sessions now report the attached session ns as the repl prompt and properly allow access to *1, *2, *3.

Show
Alex Miller added a comment - The -8 patch integrates the server startup into the Clojure runtime. It also improves validation and error-handling on the :repl commands and attached sessions now report the attached session ns as the repl prompt and properly allow access to *1, *2, *3.
Hide
Alex Miller added a comment -

Oh, and I got rid of the use of getLoopbackAddress() so this works in jdk 1.6.

Show
Alex Miller added a comment - Oh, and I got rid of the use of getLoopbackAddress() so this works in jdk 1.6.
Hide
Alex Miller added a comment -

The -10 patch defers requiring the accept ns until the socket makes a connection - this should allow a lot more freedom for the app to initialize code as needed.

Show
Alex Miller added a comment - The -10 patch defers requiring the accept ns until the socket makes a connection - this should allow a lot more freedom for the app to initialize code as needed.
Hide
Ragnar Dahlen added a comment -

Two comments on latest patch:

  • Swallowing the exception without notice in stop-server might be confusing for a user. If stop-server actually fails (but seemingly succeeds), calling start-server might fail too because port is already open, for example, but as a user it would be hard to understand why.
  • Would be helpful document return values in public API (if there is one), for example start-server/stop-server
Show
Ragnar Dahlen added a comment - Two comments on latest patch:
  • Swallowing the exception without notice in stop-server might be confusing for a user. If stop-server actually fails (but seemingly succeeds), calling start-server might fail too because port is already open, for example, but as a user it would be hard to understand why.
  • Would be helpful document return values in public API (if there is one), for example start-server/stop-server
Hide
Alex Miller added a comment -

Ragnar, good comments. I've updated the patch. That stop code was broken in a couple other ways too but should be good now.

Show
Alex Miller added a comment - Ragnar, good comments. I've updated the patch. That stop code was broken in a couple other ways too but should be good now.
Hide
Stuart Halloway added a comment -

Major stuff:

  • I don't feel like we have nailed down the requirements for the session feature, so hard to comment on that
  • attach/detach metaphor for sessions feels weird, e.g. I cannot set a dynamic var inside there and have the other session see my change. We would instead just session state query and eval-in-session APIs.
  • something needs to happen if the session you are attached to goes away out from under you

Minor stuff:

  • socket backlog arg of 0 means "use default", is there some reason 50 is better?
  • I think 'Clojure' better than 'Socket' more disinguishing as thread names
  • catch-all exception eating scares me, couldn't stop-servers call the uncaught exception handler?
  • vec + map can be replaced by mapv
  • start-client seems a very odd name to me, as this is the server side client handler
Show
Stuart Halloway added a comment - Major stuff:
  • I don't feel like we have nailed down the requirements for the session feature, so hard to comment on that
  • attach/detach metaphor for sessions feels weird, e.g. I cannot set a dynamic var inside there and have the other session see my change. We would instead just session state query and eval-in-session APIs.
  • something needs to happen if the session you are attached to goes away out from under you
Minor stuff:
  • socket backlog arg of 0 means "use default", is there some reason 50 is better?
  • I think 'Clojure' better than 'Socket' more disinguishing as thread names
  • catch-all exception eating scares me, couldn't stop-servers call the uncaught exception handler?
  • vec + map can be replaced by mapv
  • start-client seems a very odd name to me, as this is the server side client handler
Hide
Alex Miller added a comment -

Added new -12 patch with the following changes per Stu's comments:

  • In the scenario where an attached session goes away, you now lose that attachment and get an exception tell you so the next time you try to eval an expression.
  • Changed socket backlog to 0 (50 is the default, but I wasn't aware that 0 would get the default)
  • Changed thread names
  • Changed stop-server to shutdown each server in a future independently - exceptions will bubble up to default uncaught exception handler
  • Replaced vec+map with mapv
  • Changed start-client to accept-connection

Deferred discussion of session intent.

Show
Alex Miller added a comment - Added new -12 patch with the following changes per Stu's comments:
  • In the scenario where an attached session goes away, you now lose that attachment and get an exception tell you so the next time you try to eval an expression.
  • Changed socket backlog to 0 (50 is the default, but I wasn't aware that 0 would get the default)
  • Changed thread names
  • Changed stop-server to shutdown each server in a future independently - exceptions will bubble up to default uncaught exception handler
  • Replaced vec+map with mapv
  • Changed start-client to accept-connection
Deferred discussion of session intent.
Hide
Alex Miller added a comment -

The -13 patch strips out the session attach/detach/list functionality. Rescued description of those features in -12 here:

Now open a 2nd telnet client and "attach" to the session of the first client:

telnet 127.0.0.1 5555
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
user=> :repl/sessions
[{:server "repl", :client "1"} {:server "repl", :client "2"}]
user=> :repl/attach {:server "repl", :client "1"}
true
1|foo=> *ns*
#object[clojure.lang.Namespace 0x6023edaa "foo"]
1|foo=> *1
2
1|foo=> :repl/detach
true
user=> *1
true
user=> :repl/quit
Connection closed by foreign host.

Note that when the session is attached, the prompt changes, showing the client id and namespace of the attached session instead. When attached to another session, your expressions are evaluated in the context of the attached session, so you can grab the current namespace or even the *1/*2/*3 results. This is an example of how a tool could attach and be "inside" the context of the user's repl.

Show
Alex Miller added a comment - The -13 patch strips out the session attach/detach/list functionality. Rescued description of those features in -12 here: Now open a 2nd telnet client and "attach" to the session of the first client:
telnet 127.0.0.1 5555
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
user=> :repl/sessions
[{:server "repl", :client "1"} {:server "repl", :client "2"}]
user=> :repl/attach {:server "repl", :client "1"}
true
1|foo=> *ns*
#object[clojure.lang.Namespace 0x6023edaa "foo"]
1|foo=> *1
2
1|foo=> :repl/detach
true
user=> *1
true
user=> :repl/quit
Connection closed by foreign host.
Note that when the session is attached, the prompt changes, showing the client id and namespace of the attached session instead. When attached to another session, your expressions are evaluated in the context of the attached session, so you can grab the current namespace or even the *1/*2/*3 results. This is an example of how a tool could attach and be "inside" the context of the user's repl.
Hide
Alex Miller added a comment -

Add socket-change.patch for change log update.

Show
Alex Miller added a comment - Add socket-change.patch for change log update.

People

Vote (0)
Watch (8)

Dates

  • Created:
    Updated:
    Resolved: