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

The Ocsimiragen battle plan. #54

Open
Drup opened this Issue Dec 16, 2014 · 17 comments

Comments

Projects
None yet
8 participants
@Drup
Member

Drup commented Dec 16, 2014

Several people have expressed desire to help on this one, and web server internals are not really my forte, so every help is appreciated!

As I see it, here is what remains to do. I will break each task by subtasks.

  • Merge the cohttp branch
    • First merge the non-cohttp parts of the branch, that means
      • Numerous documentation improvements
      • safe-string patches
      • New interfaces that are not directly cohttp-related
    • Rebase, update to lastest versions of cohttp and clean up the commit log
    • Review carefully (I'm counting on you for this part!)
      • Ensure that all configuration are still available and remove the one that aren't.
      • Check that reverseproxy behaves as expected, in particular wrt to answer ordering. May be related to the awake parameter in #43.
  • Replace pcre by re.
    • Add functions to re if they are missing, but I think @rgrinberg and @c-cube covered that.
    • According to @balat, it's very important not to change regexp's syntax is config files, need to take care of that.
  • Implement a new Ocsipersist backend.
  • Remove what's left of ocamlnet.
  • Celebrate by spawning lots of elioms on little ARM devices.

I think that's all.

Some interfaces in the cohttp were merged as part of other PRs and the smaller the final patchset, the easier it will be to review and spot issues. I will try to work on merging the non-cohttp stuff.

cc @avsm @dinosaure @rgrinberg @vbmithr @balat @vouillon

@rgrinberg

This comment has been minimized.

Contributor

rgrinberg commented Dec 16, 2014

  • I'll volunteer to review the cohttp PR. go go @dinosaure ^_^
  • Regarding the regexp syntax compatibility. I don't think 100% compatibility is possible since Re doesn't support all of pcre. Are you sure 100% compat is necessary? Any representatives configs we can look at.
@balat

This comment has been minimized.

Member

balat commented Dec 18, 2014

I agree that it will be difficult to have 100% compatibility with pcre.
But we must be close enough, because regular expressions are used inside
configuration files.
We don't really know what people do with them (for example for sites using
user config files), and worse, it is very difficult to discover the
problems.
Most of the time a wrong page will be displayed, which can be find months
after if the page is not frequently checked.
Worse: this can cause security problems (displaying a page that should not).

We experienced this already once when syntax of back references changed
from $1 to \1 ...

However, I agree we must switch to RE.

Le Tue Dec 16 2014 at 21:51:49, Rudi Grinberg notifications@github.com a
écrit :

I'll volunteer to review the cohttp PR. go go @dinosaure
https://github.com/dinosaure ^_^

Regarding the regexp syntax compatibility. I don't think 100%
compatibility is possible since Re doesn't support all of pcre. Are you
sure 100% compat is necessary? Any representatives configs we can look at.


Reply to this email directly or view it on GitHub
#54 (comment)
.

@balat

This comment has been minimized.

Member

balat commented Dec 18, 2014

To get Eliom working on Mirage,

  • we will also need a working implementation of Ocsipersist.

And about the cohttp branch,

  • We must check carefully that all the configuration options are still available (or remove them), (for example <maxrequestbodysize>, or <maxuploadfilesize>), and also all the configuration from extendconfiguration
  • Some improvements remains to be done to allow using Server extensions like Eliom without using a configuration file. See my comment on #43 I think current code is broken. Cookie management is wrong.
  • One important thing to check is the reverse proxy. Server contains some subtle features to ensure request are answered in the order they arrive. In some (rare) cases, the order may be wrong without this (security problem). As cohttp's pipeline is very basic (compared to Server's pipeline), this should be even more rare (or even never happen) but cohttp will probably improve its pipeline one day. So we must be very careful. The awake parameter i'm speaking about in #43 may be related to this (?).
@vbmithr

This comment has been minimized.

Member

vbmithr commented Dec 18, 2014

On 18/12/2014 10:47, Vincent Balat wrote:

To get Eliom working on Mirage,
[ ] we will also need a working implementation of Ocsipersist.

Irmin.

Vincent

@smondet

This comment has been minimized.

smondet commented Dec 18, 2014

About Ocsipersist, I would recommend an OCaml-only (in memory) backend, or better: making it really optional within Eliom.
Eliom users often have another database for the rest of their application anyway.
And, when deploying in weird environments, it's annoying to have libraries that you don't use bite you in ass because of some C/system dependency.

@vbmithr

This comment has been minimized.

Member

vbmithr commented Dec 18, 2014

On 18/12/2014 15:22, Sebastien Mondet wrote:

About Ocsipersist, I would recommend an OCaml-only (in memory) backend,
or better: making it really optional within Eliom.
Eliom users often have another database for the rest of their
application anyway.
And, when deploying in weird environments, it's annoying to have
libraries that you don't use bite you in ass because of some C/system
dependency.

+1 ! dbm and sqlite should be replaced by some OCaml-only key value
store. It would be cool to plug-in Irmin though :)

Vincent

@Drup

This comment has been minimized.

Member

Drup commented Dec 18, 2014

@balat Thanks. I will add these points.

@smondet I disagree that it should be the same database as the one used in the eliom website. The session store needs to be a (fast) k/v store. SQL databases don't usually do a very good job at that (and that's not their role either).

Persistence is useful in this case (do not loose session after a reboot of the server). Irmin could work, but I think it's overkill for that. We don't need history and fancy merging feature as the one provided by Irmin. The ability to do concurrent access is also desirable.

@smondet

This comment has been minimized.

smondet commented Dec 18, 2014

I agree session storage is important, I just say it should be optional :)
(Irmin is indeed overkill)

@Drup

This comment has been minimized.

Member

Drup commented Dec 18, 2014

I'm not sure it can be optional, considering how it's used right now. @balat ?

@balat

This comment has been minimized.

Member

balat commented Dec 18, 2014

It's difficult to make it optional. I think the only way would be to create a fake ocsipersist module. But I'm quite sceptical about including this in the official release. @smondet do you never use persistent references or sessions ?

@darioteixeira

This comment has been minimized.

darioteixeira commented Dec 18, 2014

I would say that session storage is essential. You don't want to force your users to relogin just because you restarted your server...

Anyway, can anyone recommend a pure OCaml persistent kv-store? Googling returns some results (like Bitcask), but I'm not sure how mature these projects are.

@smondet

This comment has been minimized.

smondet commented Dec 18, 2014

Not every website has “users” :)

Also, Eliom can be used to build local GUIs that run in headless browsers.
Or used to build mobile apps (FirefoxOS style).

Also, we should be able to keep session info in a "non-local" database. For example if you have 20 webservers serving the same application (like mirage unikernels with no R/W filesystem at all).

@Drup

This comment has been minimized.

Member

Drup commented Dec 18, 2014

I agree with the last point, but that should be possible with a new ocsipersist interface.

@Drup

This comment has been minimized.

Member

Drup commented Dec 19, 2014

I moved ocsipersist out of ocsigenserver. Let's move the discussion specific to ocsipersist to ocsigen/ocsipersist#2.

@vouillon

This comment has been minimized.

Member

vouillon commented Mar 10, 2015

I have rebased the cohttp branch (new branch "cohttp").

@Drup

This comment has been minimized.

Member

Drup commented Aug 22, 2016

Partially. The work on the cohttp branch was stalled, partially because I'm not working on it anymore, partially because the it is actually quite difficult ...

We haven't given up on the whole thing, though. :)

@qrntz

This comment has been minimized.

qrntz commented Aug 22, 2016

@Drup, sorry; I've deleted my comment because I found yours at mirage/mirage#569 (comment) :-)

Didn't expect you to answer so quickly.

Anyway, thanks for letting me know.

@balat balat added the Epic label Jun 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment