Bundle for making HTTP GETrequests #369

pengstrom · 2016-04-11T11:41:25Z

There were no Encore bindings to the C functions for network programming. I have written a convenient wrapper around some C code for making http requests able to receive an arbitrary sized response.

Testing is a little weird since we don't have a development web server of our own. Instead, I request www.httpbin.org to test different return codes.

To make a request, import Request and use the GET function to make a request

let r = new Request();
let res = get r.GET("www.httpbin.org", 80, "/ip");
print("Response is:\n{}\n", res.raw);

I have only implemented the GET functionality, and the response object only contains the raw request. I plan on enabling more methods and convenient getters in the response object.

Using network programming in C, we wrap the functionality in an active Encore class.

A rudimetary class Response with a single field for the raw respose is used as the resturn type of Response.GET Using the website 'www.httpbin.org' we test responses of statuses 200, 300, 400 and 500.

EliasC · 2016-04-11T13:18:30Z

bundles/prototype/Request/Request.enc

+
+    // Allocate the buffer and result
+    int result_size = buffer_size + 1;
+    char *buffer = malloc(buffer_size);


Could this be allocated on the stack? Now it looks like this memory will leak!

EliasC · 2016-04-11T22:26:30Z

@pengstrom: See #373 for methods that might be useful for your tests!

print "/status/".concatenate(string_from_int(42));

kikofernandez · 2016-04-12T05:00:44Z

As a side note, the PonyRT has support for Async IO. I read an article where someone mentioned that it is pretty good (I cannot find the article now...)
In any case, I cannot wait to start creating my own server! :)

Unlike encore_alloc, encore_realloc doesn't zero the memory on resizing (into larger) and you have to do that manually using memset.

pengstrom · 2016-04-16T19:49:37Z

I've updated my branch with your kind suggestions.

I'm now using the native conversion functions for strings, and the memory is (hopefully) garbage collected.

In addition, a function encore_realloc(pony_ctx_t *ctx, void* p, size_t s) (a wrapper around pony_realloc) was written for dynamic resizing. I'd like some ideas how this could be zeroed, since arithmetic on void pointers is not allowed.

EliasC · 2016-04-17T07:49:29Z

bundles/prototype/Request/Request.enc

+#include <netinet/in.h> /* struct sockaddr_in, struct sockaddr */
+#include <netdb.h> /* struct hostent, gethostbyname */
+
+#include <encore.h> /* encore_alloc, encore_realloc */


The file compiles for me even if I remove stdio.h, stdlib.h, string.h and encore.h.

I guess it's included elsewhere up the chain. But isn't it good to be explicit about dependencies?

It doesn't hurt having them there I guess. At least encore.h seems redundant though, as it must be present for the Encore code to compile. I have no strong feelings about removing or keeping them.

EliasC · 2016-04-17T08:13:14Z

Only minor things left, then I think this is mergable!

Also used native string functions for concatenating stings.

EliasC · 2016-04-17T15:10:41Z

I'm in favour of merging this now! Will do so tomorrow if no one objects.

albertnetymk · 2016-04-17T22:06:05Z

I don't think it's needed for the identical comment to appear in both header and source file.

There are trailing space in bundles/prototype/Request/Request.enc.

For the embedded C code, clang -Weverything gives a few warnings; the ones with noreturn, losing precision on ssize_t to int should be addressed, IMO.

getRequest's naming seems odd, for it returns response, not request. Maybe I misunderstood sth.

It's unclear to me why strcpy is needed in GET of Request.

I didn't get any error on running RequestTest. @EliasC Are you referring to this PR with the crashing you mentioned on slack?

EliasC · 2016-04-18T07:05:47Z

@albertnetymk: I'm getting errors if I increase the number of iterations in main. Some of the errors is fixed by #384, but I'm still seeing GC crashes (which go away with --ponythreads 1).

albertnetymk · 2016-04-18T09:06:34Z

@EliasC I can't reproduce the failure with repeat i <- 100 around the original code. What code did you use to see the crash?

EliasC · 2016-04-18T09:12:22Z

@albertnetymk: I see the crash with for n in [200..500 by 10].

albertnetymk · 2016-04-18T09:19:53Z

@EliasC I can confirm it on my box. Looks like a data-race caused bug from the error msg in gdb.

EliasC · 2016-04-18T09:21:01Z

@albertnetymk: 👍

Also, it goes away with --ponythreads 1.

albertnetymk · 2016-04-18T22:06:06Z

The crashing is concluded to be caused by misusing obsolete ctx after work stealing. The PR is not to be blamed for the crashing.

pengstrom · 2016-04-19T14:39:45Z

Thanks for your feedback!

@albertnetymk

For the embedded C code, clang -Weverything gives a few warnings; the ones with noreturn, losing precision on ssize_t to int should be addressed, IMO.

I'm only losing precision with the functions write and read which are limited to 4096 bytes. So if I'm not mistaken, it should not be an issue.

For the noreturn, is it enough if I add the attribute to the error function, or should I write it in another way?

The comment is kept in the header

albertnetymk · 2016-04-19T14:46:22Z

So if I'm not mistaken, it should not be an issue.

True, but it would be surprising to see that the program starts to behave strangely with a larger buffer size. The benefit of using a macro for buffer size is that the rest of the program can be kept intact.

For the noreturn, is it enough if I add the attribute to the error function

Think so.

pengstrom · 2016-04-19T15:05:17Z

@albertnetymk
You're right. I'll make the function use ssize_t throughout.

albertnetymk · 2016-04-19T22:43:11Z

Looks good.
Two very minor comments:

indentation is inconsistent 2 spaces vs 4 spaces
exceeding 80 columns

Can be merged as it is. I would suggest to squash it into one commit on merging, unless the author of this PR plans to do a rebase to tidy the history a bit.

pengstrom · 2016-04-20T19:32:21Z

Go ahead and squash!

pengstrom added 3 commits April 10, 2016 22:38

Wrote wrapper around c sockets

33b50b2

Using network programming in C, we wrap the functionality in an active Encore class.

Adde test request

f20c615

Refactored return type into Response class and added tests

8d8770c

A rudimetary class Response with a single field for the raw respose is used as the resturn type of Response.GET Using the website 'www.httpbin.org' we test responses of statuses 200, 300, 400 and 500.

TobiasWrigstad added the ready for review label Apr 11, 2016

EliasC reviewed Apr 11, 2016
View reviewed changes

pengstrom added 6 commits April 13, 2016 15:14

Function encore_realloc for dynamic memory reallocation

13a1fed

Unlike encore_alloc, encore_realloc doesn't zero the memory on resizing (into larger) and you have to do that manually using memset.

Request get method

5beb7f2

Merge branch 'development' into pengstrom-request-tmp

6b752cf

Changed return value back to Response for GET

21272cc

Merge branch 'pengstrom-request-tmp' into pengstrom-request-get

ddc3db3

Implemented native string conversion in place of ad hoc C

099256d

EliasC reviewed Apr 17, 2016
View reviewed changes

Store local arrays on the stack instead of the heap

5f27734

Also used native string functions for concatenating stings.

Removed identical comment from source file

24e85cc

The comment is kept in the header

Renamed getRequest to more semantically correct getResponse

a4620ef

Changed types to avoid precision loss

3332ab1

albertnetymk merged commit a95128e into parapluu:development Apr 20, 2016

TobiasWrigstad removed the ready for review label Apr 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bundle for making HTTP GETrequests #369

Bundle for making HTTP GETrequests #369

pengstrom commented Apr 11, 2016

EliasC Apr 11, 2016

EliasC commented Apr 11, 2016

kikofernandez commented Apr 12, 2016

pengstrom commented Apr 16, 2016

EliasC Apr 17, 2016

pengstrom Apr 17, 2016

EliasC Apr 17, 2016

EliasC commented Apr 17, 2016

EliasC commented Apr 17, 2016

albertnetymk commented Apr 17, 2016

EliasC commented Apr 18, 2016

albertnetymk commented Apr 18, 2016

EliasC commented Apr 18, 2016

albertnetymk commented Apr 18, 2016

EliasC commented Apr 18, 2016

albertnetymk commented Apr 18, 2016

pengstrom commented Apr 19, 2016

albertnetymk commented Apr 19, 2016

pengstrom commented Apr 19, 2016

albertnetymk commented Apr 19, 2016

pengstrom commented Apr 20, 2016

Bundle for making HTTP GETrequests #369

Bundle for making HTTP GETrequests #369

Conversation

pengstrom commented Apr 11, 2016

EliasC Apr 11, 2016

Choose a reason for hiding this comment

EliasC commented Apr 11, 2016

kikofernandez commented Apr 12, 2016

pengstrom commented Apr 16, 2016

EliasC Apr 17, 2016

Choose a reason for hiding this comment

pengstrom Apr 17, 2016

Choose a reason for hiding this comment

EliasC Apr 17, 2016

Choose a reason for hiding this comment

EliasC commented Apr 17, 2016

EliasC commented Apr 17, 2016

albertnetymk commented Apr 17, 2016

EliasC commented Apr 18, 2016

albertnetymk commented Apr 18, 2016

EliasC commented Apr 18, 2016

albertnetymk commented Apr 18, 2016

EliasC commented Apr 18, 2016

albertnetymk commented Apr 18, 2016

pengstrom commented Apr 19, 2016

albertnetymk commented Apr 19, 2016

pengstrom commented Apr 19, 2016

albertnetymk commented Apr 19, 2016

pengstrom commented Apr 20, 2016