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

[WIP] Encapsulate applications #511

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

[WIP] Encapsulate applications #511

wants to merge 18 commits into from

Conversation

sdogruyol
Copy link
Member

@sdogruyol sdogruyol commented Dec 17, 2018

Building on top of @straight-shoota's initial work 👍

This currently builds and runs with Crystal 0.27.0

@sdogruyol
Copy link
Member Author

sdogruyol commented Dec 26, 2018

This is still WIP. I'm still trying to come up with some architectural decisions regarding the API.

Option 1: We can allow each application inheriting from Kemal::Application to have its' own everything including HTTP::Server. If so what should be the default behavior about ports? One can use reuse_port: true to reuse the same port and spawn multiple servers.

Sample code:

class MyApp < Kemal::Application
  get "/" do
    "Hello from 1"
  end
end

class OtherApp < Kemal::Application
  get "/other" do |env|
    pp "2"
    "Hello from 2!"
  end
end

spawn { OtherApp.run(port: 3000, reuse_port: true )}
MyApp.run(port: 3000, reuse_port: true)

Notice the spawn, this is needed to spawn the first server without blocking the main thread. It's a bit hacky 🤔

Option 2: We can only have singletons of RouteHandler, FilterHandler, WebsocketHandler and only one HTTP::Server through the whole execution cycle. This is pretty much the same with the current Kemal architecture.

With this approach, we'll still need to have something like Kemal.run to start the HTTP::Server. We'll also going to need something like mount to actually add the application.

Sample code

app1 = MyApp.new

app1.get "/admin" do |env|
  "app1"
end

app2 = OtherApp.new

app2.get "/" do |env|
  "app2"
end


Kemal.mount([app1, app2])
Kemal.run

Eventually we can improve mount with specific paths

Kemal.mount("/", app1)
Kemal.mount("/admin", app2)

@oguzbilgic
Copy link

oguzbilgic commented Dec 26, 2018

Keep up the great work. Here are my two cents as a new crsytal user;

Both options can be possible at the same time. Inherit Kemal::Application to create encapsulated routes (or apps). Either use/run/start them separately or glue/mount them all together and start as a single fiber.

class ApiApp < Kemal::Application
...
end

# Start just the api
ApiApp.run 

# or combine multiple apps together 
Kemal.run ApiApp, DashboardApp 

# or mount apps to paths
Kemal.run do |mount|
  mount '/api', ApiApp
  mount '/admin', DashboardApp
end

@straight-shoota
Copy link
Contributor

Option 2 doesn't provide much benefit. I'm not sure if the whole encapsulating process would really be worth it if only this was the output.

A use case for Option 1 is to essentially run multiple servers from one executable, for example providing a separate administration interface which is only accessible via a separate port and can thus be easily secured by making it only locally accessible.
Mounting an application into another one is also possible with this approach. Instead of running the second application in it's own server, it's handler chain could just be inserted into the other application.
This would still need some additional work obviously, but it should be a logical enhancement.

Btw. using reuse_port is a really bad example, it doesn't make any sense. With two applications listening on the same port, incoming requests would alternate between both HTTP::Server instances, meaning only every other request will be able to access /other (the same goes for /).

@vladfaust
Copy link

There is https://github.com/vladfaust/http-multiserver.cr which allows to bind multiple servers to a single socket ☝️

@mamantoha
Copy link
Contributor

mamantoha commented Dec 27, 2018

Nice! 🎉

This code doesn't work:

class MyApp < Kemal::Application
  before_all do |env|
    env.set "key", "value"
  end

  get "/" do |env|
    "Hello Kemal!"
  end
end

app = MyApp.new
app.run

While this one works:

class MyApp < Kemal::Application
  get "/" do |env|
    "Hello Kemal!"
  end
end

app = MyApp.new
app.before_all do |env|
  env.set "key", "value"
end
app.run

@straight-shoota
Copy link
Contributor

@vladfaust No offense, but HTTP::Multiserver doesn't look very well-thought-out. Why does it even need to inherit HTTP::Server? This breaks concerns. HTTP::Server and RequestProcessor are for multiplexing socket connections onto one handler chain. Mounting other handler chains can be easily adopted directly inside a handler chain. This would make it even possible to recursively mount applications into each other (if you'd ever need that).

@plainas
Copy link

plainas commented Jan 4, 2019

Great to see this happening. We've talk about this before and this is, in my opinion, a killer feature.

As an user that have asked for this feature, I would like to share the following thought:

The ability to hardcode a few apps and run them on different paths as shown in the examples above is nice and all, but in the end is just a nicer api for what was already possible. Albeit not through a clean api.

The real added functionality is being ably to dynamically mount applications on runtime. And, the awesomeness, would be the ability to do that a not-so-small scale. Say, mounting hundreds of applications. This is not possible ATM.

For example, say I have a SaaS and want to provide a rest api per user. I could, under the limits of what's reasonable, retrieve the user details from my database, create an app instance for each and run them.

Whatever the api is going to look like will be a matter of style/preference. Ultimately this is a less important detail. I would suggest putting the focus on making a larger number of application instances not only doable but performant.
Routing at the root url namespace should be fast rather than full featured.

It is important to be clear about the amount of apps that would be reasonable to mount: 10? 100? 1000? more? Personally, I don't see much point in doing it for less than say, 50. Sounds extreme but if you think about it, if you want to mount 10 apps, you could easily do it with a workaround like using prefixes as namespaces for your routes, for example with the help of a macro.
Aiming for 1000 would be awesome. I don't think it's such a crazy idea.

Keep up the good work.

@bararchy
Copy link

@straight-shoota & @sdogruyol any updates on this? I would love seeing this happening :)

@Daniel-Worrall
Copy link

Daniel-Worrall commented Aug 31, 2019

nice work, looking forward to the release.
A note on Signal::INT.trap, I believe this will need to be amended to handle exiting multiple servers gracefully as it will currently overwrite the previous trap when a new server is created.

Could either have extendable traps in Crystal, or perhaps a class variable that stores new Kemal apps and the trap will run through the list so even if a new one is created, the same code is written and it handles both.

@Dan-Do
Copy link

Dan-Do commented Jun 2, 2021

Does kemal support preview_mt now?

@xendk
Copy link
Contributor

xendk commented Sep 23, 2023

Would be nice if this got some traction again.

My personal experience on a very small project is that while the un-encapsulatedness of the current implementation gets you off the ground very fast, you quickly run into some state or dependency that need to be shared between routes. This can be solved by top level variables, Singletons and other means, but it's very mind-bending for someone coming from more conventional OO languages.

I do like the concept of defining a class and then calling run on that. It's not that it's much more work and it has the added benefit of making state (instance vars) and dependencies (injection) obvious for the new-to-Crystal-but-experienced-in-OO user (and I'd say not too difficult to grasp for the total newbie either).

Just my two cents.

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.

10 participants