-
Notifications
You must be signed in to change notification settings - Fork 65
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
Added UNIX support to nitcorn #2709
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I like the idea of implementing UNIX socket in its own module, I see a lot of duplicated code. You can instead use intrude import reactor
to access internal services not exposed in the public nitcorn API.
With this implementation, can you run a server listening on both TCP and UNIX sockets?
vh.routes.add new Route(null, new FileServer("www/hello_world/")) | ||
|
||
# Set server in UNIX mode | ||
vh.unix("/tmp/example.sock") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you just add this line to an existing example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since unix methods are on a separate module, are you sure you don't want to keep this example in a separate file ?
lib/nitcorn/unix.nit
Outdated
# Handle to retreive the `HttpFactory` on config change | ||
private var factory: HttpFactory is noinit | ||
|
||
private init with_factory(factory: HttpFactory) do self.factory = factory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this already in reactor
?
lib/nitcorn/unix.nit
Outdated
# Configuration of this server | ||
# | ||
# It should be populated after this object has instanciated | ||
redef var config = new ServerConfig.with_factory(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
lib/nitcorn/unix.nit
Outdated
var unix_path = "" is optional, writable | ||
# # Has `self` been registered by `listen_on`? | ||
private var registered = false | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please double check the style here.
- Added new module `nitcorn::unix` to add unix support - Added tests&example code for new module `unix` - Multiple dirty redifinitions of reactor's class for unix support (because of dirty redefinity of `Sys` in reactor) - Implementation of unix support for libevent added in PR nitlang#2708 Signed-off-by: Matthieu Le Guellaut <leguellaut.matthieu@gmail.com>
UPDATE
|
**DO NOT MERGE BEFORE PR #2708** As requested in #2678 - Added new module `nitcorn::unix` to add unix support - Added tests&example code for new module `unix` - Multiple dirty redifinitions of reactor's class for unix support (because of dirty redefinity of `Sys` in reactor) - Implementation of unix support for libevent added in PR #2708 Pull-Request: #2709 Reviewed-by: Alexis Laferrière <alexis.laf@xymus.net>
Merged as #2726 |
DO NOT MERGE BEFORE PR #2708
As requested in #2678
nitcorn::unix
to add unix supportunix
Sys
in reactor)