RejectedSoftware Forums

Sign up

Pages: 1 2

First impressions

So, here are some first impressions/issues that came up immediately.
Perhaps I'm doing it wrong, and if so, I'd appreciate guidance.

1) req.form, req.query, etc, maps throw if you try to access an element that isn't there. This leads to endless spam like this:

 if("xys" in req.form)
 {
   string xyz = req.form["xyz"];
   ...
 }

or

 string xyz = "xyz" in req.form ? req.form["xyz"] : null;

That's ugly and annoying. Is that really the accepted pattern?

Surely:

 string xyz = req.form["xyz"];

...is better, and xyz should just be assigned null if not present?

2) HTTPServerRequest has no way to carry user data. URL router is a linear process, and functions like 'check login' that need to look up a user profile should be able to carry that profile across into the following handler. Otherwise the actual handler just has to look up the same user again.
Ie, there is no way to carry state through the URL route. I have a few situations of this sort where it would be simpler and more efficient to carry some state.

3) Fiber Local Storage. Since TLS isn't reliable in the vibe.d environment, an explicit solution should be offered, otherwise users are likely to try using TLS and run into unexpected problems...

Re: First impressions

1.2) It's pretty lame that all the examples on the website do stuff like:

res.redirect("/users/"~req.post["user"]);

With no comment to note that the application will CRASH if post["user"] is not present. My presumption was that it would just be handled as a null string.

Re: First impressions

On Fri, 26 Jul 2013 11:19:39 GMT, Manu Evans wrote:

So, here are some first impressions/issues that came up immediately.
Perhaps I'm doing it wrong, and if so, I'd appreciate guidance.

1) req.form, req.query, etc, maps throw if you try to access an element that isn't there. This leads to endless spam like this:

 if("xys" in req.form)
 {
   string xyz = req.form["xyz"];
   ...
 }

or

 string xyz = "xyz" in req.form ? req.form["xyz"] : null;

That's ugly and annoying. Is that really the accepted pattern?

Surely:

 string xyz = req.form["xyz"];

...is better, and xyz should just be assigned null if not present?

You also have req.form.get("xyz", null) which does exactly that (see http://dlang.org/hash-map.html). Since this is standard D AA behavior, I didn't want to deviate from that potentially creating confusion.

2) HTTPServerRequest has no way to carry user data. URL router is a linear process, and functions like 'check login' that need to look up a user profile should be able to carry that profile across into the following handler. Otherwise the actual handler just has to look up the same user again.
Ie, there is no way to carry state through the URL route. I have a few situations of this sort where it would be simpler and more efficient to carry some state.

The HTTPServerRequest.params field is meant for such cases where data needs to be passed down the request handler chain. Unfortunately for historic reasons it is a string[string] map, although it should rather be Variant[string]. The problem here is just finding a transition that doesn't break all existing code (adding an additional field would be the easy option, but is also kind of ugly unless it actually has a better name...).

3) Fiber Local Storage. Since TLS isn't reliable in the vibe.d environment, an explicit solution should be offered, otherwise users are likely to try using TLS and run into unexpected problems...

There is vibe.core.core.setTaskLocal and getTaskLocal for task local storage. They use a Variant[string] map internally, which is not ideal regarding performance (incl. memory allocations). But a really good solution would probably require some form of compiler support.... well thinking about it, maybe not. It should be possible to create a TaskLocal!T template which registers a slot in a linear per-task memory area.

Task local storage is of course the obvious alternative for 2) until the params field can carry arbitrary types.

BTW 2) and 3) have come up now a number of times, so I'll see how their visibility can be improved in the documentation.

On Fri, 26 Jul 2013 11:26:27 GMT, Manu Evans wrote:

1.2) It's pretty lame that all the examples on the website do stuff like:

res.redirect("/users/"~req.post["user"]);

With no comment to note that the application will CRASH if post["user"] is not present. My presumption was that it would just be handled as a null string.

It should actually be caught by the HTTP server and result in a 500 error. However, on Windows and especially in the Visual Studio debugger, any kind of exception can sometimes be unrecoverable due to some issues with the injected exception handler or other issues that I didn't really track down. Maybe that is what you've seen? Anyway, proper input verification would surely be in order for the examples.

Re: First impressions

On Fri, 26 Jul 2013 12:12:11 GMT, Sönke Ludwig wrote:

On Fri, 26 Jul 2013 11:19:39 GMT, Manu Evans wrote:

So, here are some first impressions/issues that came up immediately.
Perhaps I'm doing it wrong, and if so, I'd appreciate guidance.

1) req.form, req.query, etc, maps throw if you try to access an element that isn't there. This leads to endless spam like this:

 if("xys" in req.form)
 {
   string xyz = req.form["xyz"];
   ...
 }

or

 string xyz = "xyz" in req.form ? req.form["xyz"] : null;

That's ugly and annoying. Is that really the accepted pattern?

Surely:

 string xyz = req.form["xyz"];

...is better, and xyz should just be assigned null if not present?

You also have req.form.get("xyz", null) which does exactly that (see http://dlang.org/hash-map.html). Since this is standard D AA behavior, I didn't want to deviate from that potentially creating confusion.

Ah right, I knew I must be missing something obvious! (I've never used a builtin AA before, I didn't spot the get() method.

2) HTTPServerRequest has no way to carry user data. URL router is a linear process, and functions like 'check login' that need to look up a user profile should be able to carry that profile across into the following handler. Otherwise the actual handler just has to look up the same user again.
Ie, there is no way to carry state through the URL route. I have a few situations of this sort where it would be simpler and more efficient to carry some state.

The HTTPServerRequest.params field is meant for such cases where data needs to be passed down the request handler chain. Unfortunately for historic reasons it is a string[string] map, although it should rather be Variant[string]. The problem here is just finding a transition that doesn't break all existing code (adding an additional field would be the easy option, but is also kind of ugly unless it actually has a better name...).

I see... Well I'll leave it with you.

3) Fiber Local Storage. Since TLS isn't reliable in the vibe.d environment, an explicit solution should be offered, otherwise users are likely to try using TLS and run into unexpected problems...

There is vibe.core.core.setTaskLocal and getTaskLocal for task local storage. They use a Variant[string] map internally, which is not ideal regarding performance (incl. memory allocations). But a really good solution would probably require some form of compiler support.... well thinking about it, maybe not. It should be possible to create a TaskLocal!T template which registers a slot in a linear per-task memory area.

Yeah, I think a good library implementation is possible. I'll use your API for now.

Task local storage is of course the obvious alternative for 2) until the params field can carry arbitrary types.

Indeed, although they are subtly different in feel/usage. I'll see how I go with that.

BTW 2) and 3) have come up now a number of times, so I'll see how their visibility can be improved in the documentation.

Cool.

1.2) It's pretty lame that all the examples on the website do stuff like:

res.redirect("/users/"~req.post["user"]);

With no comment to note that the application will CRASH if post["user"] is not present. My presumption was that it would just be handled as a null string.

It should actually be caught by the HTTP server and result in a 500 error. However, on Windows and especially in the Visual Studio debugger, any kind of exception can sometimes be unrecoverable due to some issues with the injected exception handler or other issues that I didn't really track down. Maybe that is what you've seen? Anyway, proper input verification would surely be in order for the examples.

Yeah, well this is also just a reflection of my n00b-ness. Although I reckon anyone new to D will make the same mistake!

And now a new issue that has just come up...

My application that makes HTTP requests seems to lock up vibe.d. It seems to sit there waiting for more data over the socket, even though it's received all that it's going to.
Then my app disconnects the socket and vibe.d dies inside:

auto reqln = cast(string)stream.readLine(MaxHTTPHeaderLineLength, "\r\n", alloc);

The thing that caught my eye immediately was the "\r\n" thing. That's not the HTTP standard is it?
My app sends the HTTP request with "\n" separating the lines. Other HTTP servers accept my requests just fine, but vibe.d locks up... weird.

I tried changing my app to use "\r\n" line separators instead of "\n", and vibe.d started working as expected...

what's the deal?

Re: First impressions

On Fri, 26 Jul 2013 12:12:11 GMT, Sönke Ludwig wrote:

You also have req.form.get("xyz", null) which does exactly that (see http://dlang.org/hash-map.html). Since this is standard D AA behavior, I didn't want to deviate from that potentially creating confusion.

Upon further consideration, I'm not sure I agree this is a good idea.
I tend to think web dev should be as friction-less as possible, and this is a rather pointless point of friction...
I can see where you're coming from, but I'm not sure I agree. Getting form/url args is a fundamental operation.

Perhaps it might be interesting if you supported opDispatch? I found myself even getting annoyed at having to type req.form["xyz"]. The whole [""] thing is unnatural to type quickly (at least for me), and it looks pretty noisy. 'req.form.xyz' would be pretty nice!

Re: First impressions

On Fri, 26 Jul 2013 12:43:54 GMT, Manu Evans wrote:

And now a new issue that has just come up...

My application that makes HTTP requests seems to lock up vibe.d. It seems to sit there waiting for more data over the socket, even though it's received all that it's going to.
Then my app disconnects the socket and vibe.d dies inside:

auto reqln = cast(string)stream.readLine(MaxHTTPHeaderLineLength, "\r\n", alloc);

The thing that caught my eye immediately was the "\r\n" thing. That's not the HTTP standard is it?
My app sends the HTTP request with "\n" separating the lines. Other HTTP servers accept my requests just fine, but vibe.d locks up... weird.

I tried changing my app to use "\r\n" line separators instead of "\n", and vibe.d started working as expected...

what's the deal?

Again, my bad. I just reviewed the spec, it does say '\r\n' >_<
I'm amazed I got away with this for so long. Apparently it's not a very rigid spec.
At least Google AppEngine and Apache support lone '\n' separators.

Re: First impressions

Am 26.07.2013 14:52, schrieb Manu Evans:

On Fri, 26 Jul 2013 12:12:11 GMT, Sönke Ludwig wrote:

You also have req.form.get("xyz", null) which does exactly that (see http://dlang.org/hash-map.html). Since this is standard D AA behavior, I didn't want to deviate from that potentially creating confusion.
Upon further consideration, I'm not sure I agree this is a good idea.
I tend to think web dev should be as friction-less as possible, and this is a rather pointless point of friction...
I can see where you're coming from, but I'm not sure I agree. Getting form/url args is a fundamental operation.

Perhaps it might be interesting if you supported opDispatch? I found myself even getting annoyed at having to type req.form["xyz"]. The whole [""] thing is unnatural to type quickly (at least for me), and it looks pretty noisy. 'req.form.xyz' would be pretty nice!

That would indeed be nice, but the question is which of the two is
usually desired?

string opDispatch(string field)() const {
    return m_aa.get(field, null);
}

or

string opDispatch(string field)() const {
    auto p = field in m_aa;
    enforceHTTP(p !is null, HTTPStatus.badRequest,
        "Request missing form field "~field);
    return *p;
}

One related thing you might want to look at is the vibe.http.form
module, which automatically maps form fields to function parameters. It
is not yet in a final state though and I think has some documentation
errors. But in theory that would be the most friction-less way to handle
form requests. I'll start using it for a project in the near future and
will improve it along the way.

Re: First impressions

Am 26.07.2013 14:59, schrieb Manu Evans:

On Fri, 26 Jul 2013 12:43:54 GMT, Manu Evans wrote:

And now a new issue that has just come up...

My application that makes HTTP requests seems to lock up vibe.d. It seems to sit there waiting for more data over the socket, even though it's received all that it's going to.
Then my app disconnects the socket and vibe.d dies inside:

auto reqln = cast(string)stream.readLine(MaxHTTPHeaderLineLength, "\r\n", alloc);

The thing that caught my eye immediately was the "\r\n" thing. That's not the HTTP standard is it?
My app sends the HTTP request with "\n" separating the lines. Other HTTP servers accept my requests just fine, but vibe.d locks up... weird.

I tried changing my app to use "\r\n" line separators instead of "\n", and vibe.d started working as expected...

what's the deal?

Again, my bad. I just reviewed the spec, it does say '\r\n' >_<
I'm amazed I got away with this for so long. Apparently it's not a very rigid spec.
At least Google AppEngine and Apache support lone '\n' separators.

I have to be very grateful that you made that mistake, though! ;) I've
always wondered why I sometimes get requests that resulted in a read
timeout at exactly that place (without any means to reproduce that in a
debugger). I guess now I have an explanation.

It will be slightly tricky to support a bare '\n', because readLine
builds on top of a generic readUntil function, which would need to be
modified to support multiple search terms. I'll open a ticket for now.

Re: First impressions

On Fri, 26 Jul 2013 15:51:59 +0200, Sönke Ludwig wrote:

Am 26.07.2013 14:52, schrieb Manu Evans:

On Fri, 26 Jul 2013 12:12:11 GMT, Sönke Ludwig wrote:

You also have req.form.get("xyz", null) which does exactly that (see http://dlang.org/hash-map.html). Since this is standard D AA behavior, I didn't want to deviate from that potentially creating confusion.
Upon further consideration, I'm not sure I agree this is a good idea.
I tend to think web dev should be as friction-less as possible, and this is a rather pointless point of friction...
I can see where you're coming from, but I'm not sure I agree. Getting form/url args is a fundamental operation.

Perhaps it might be interesting if you supported opDispatch? I found myself even getting annoyed at having to type req.form["xyz"]. The whole [""] thing is unnatural to type quickly (at least for me), and it looks pretty noisy. 'req.form.xyz' would be pretty nice!

That would indeed be nice, but the question is which of the two is
usually desired?

string opDispatch(string field)() const {
    return m_aa.get(field, null);
}

or

string opDispatch(string field)() const {
    auto p = field in m_aa;
    enforceHTTP(p !is null, HTTPStatus.badRequest,
        "Request missing form field "~field);
    return *p;
}

If you asked me, I would say the former, since you can perform that enforce in the outer scope yourself if you like.
I guess I'm having a lot of trouble visualising how to write nice looking code with extensive use of exceptions.
If simple form handling logic is potentially throw-ing all over the place, what is the typical structure/flow to handle errors gracefully?
Are there any idiomatic examples of this sort of code?

I must confess, when I write C++, I always supply the compiler option to disable the exception handler ;)
I find comparing strings against null to be far simpler and more readable... but maybe that's just me.

Re: First impressions

On Fri, 26 Jul 2013 14:40:47 GMT, Manu Evans wrote:

(...)

If you asked me, I would say the former, since you can perform that enforce in the outer scope yourself if you like.
I guess I'm having a lot of trouble visualising how to write nice looking code with extensive use of exceptions.
If simple form handling logic is potentially throw-ing all over the place, what is the typical structure/flow to handle errors gracefully?
Are there any idiomatic examples of this sort of code?

I must confess, when I write C++, I always supply the compiler option to disable the exception handler ;)
I find comparing strings against null to be far simpler and more readable... but maybe that's just me.

I'm right there with you. I also never used exceptions in C++ for various reasons and took a long time until I accepted the fact that so many things in D's runtime and standard library depend on them that it doesn't make sense to actively avoid them (however, I still fear I might regret that when looking at the state of non-x86 platforms). Funnily, the thing that made me actually start using them was to make the code look cleaner. I think I looked at a hand written D parser and was impressed how clean that code looked compared to my already clean C-style code.

The good thing is that usually you don't have to care about those exceptions, as long as important cleanup code properly uses scope(exit) or similar means. I have a few places like the following, though, where an explicit try-catch is used:

void handleSubmittedForm(HTTPServerRequest req, HTTPServerResponse res)
{
	try {
		// read form values
		// validate values
		// perform action
		res.redirect("http://page/displaying/results");
	} catch (Exception e) {
		req.params["error"] = e.msg;
		// prints the error message
		renderFormPage(req, res);
	}
}

It helps a lot to keep the error handling code short and readable. But on the other hand you are absolutely right that if opDispatch throws an exception, the get-like behavior cannot be achieved in a sane way ("insane" meaning catching the exception and using a default value). On the other hand I don't like that failures could easily go undetected if the return value has to be explicitly checked. What do you think of a vibe.http.form-like approach? I somehow like the idea of having a low-level API and a higher level layer for maximum convenience to sidestep such issues.

Pages: 1 2