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

Add a rust implementation. #42

Merged
merged 1 commit into from
Oct 13, 2015
Merged

Add a rust implementation. #42

merged 1 commit into from
Oct 13, 2015

Conversation

RoPP
Copy link
Contributor

@RoPP RoPP commented Aug 8, 2015

No description provided.

@moredip
Copy link
Contributor

moredip commented Aug 9, 2015

Thanks for the contribution. Unfortunately it seems to be failing the specs at the moment: http://www.todobackend.com/specs/index.html?https://todo-backend-rustful.herokuapp.com.

@RoPP
Copy link
Contributor Author

RoPP commented Aug 9, 2015

That's strange, it looks ok for me :
capture d ecran de 2015-08-09 08-51-41

@moredip
Copy link
Contributor

moredip commented Aug 9, 2015

Interesting. Maybe this is only failing in certain browsers. I'm using Chrome and when I run the tests the server returns "null" as the body for the todos that are created. Even trying to retrieve them via curl still results in a null body:

⋙ curl -vv http://todo-backend-rustful.herokuapp.com/18
* Hostname was NOT found in DNS cache
*   Trying 79.125.111.225...
* Connected to todo-backend-rustful.herokuapp.com (127.0.0.1) port 80 (#0)
> GET /18 HTTP/1.1
> User-Agent: curl/7.37.1
> Host: todo-backend-rustful.herokuapp.com
> Accept: */*
>
< HTTP/1.1 200 OK
< Connection: keep-alive
< Content-Type: application/json; charset=utf-8
< Date: Sun, 09 Aug 2015 14:37:23 GMT
* Server rustful is not blacklisted
< Server: rustful
< Access-Control-Allow-Headers: content-type
< Access-Control-Allow-Methods: DELETE, OPTIONS, PATCH, GET
< Access-Control-Allow-Origin: *
< Content-Length: 4
< Via: 1.1 vegur
<
* Connection #0 to host todo-backend-rustful.herokuapp.com left intact
null

I'm wondering whether the POST used by Chrome to create these todos is somehow different from the one used by Firefox, and that's causing your implementation to create the todos differently. Here's what Chrome is posting to create the todos during tests:

POST / HTTP/1.1
Host: todo-backend-rustful.herokuapp.com
Connection: keep-alive
Content-Length: 19
Accept: text/plain, */*; q=0.01
Origin: http://www.todobackend.com
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.125 Safari/537.36
Content-Type: application/json
Referer: http://www.todobackend.com/specs/index.html?https://todo-backend-rustful.herokuapp.com
Accept-Encoding: gzip, deflate
Accept-Language: en-US,en;q=0.8

{"title":"my todo"}

What's really weird is if I try to reproduce this by using what Chrome claims is the same request but exported as a curl command then everything works as expected:

⋙ curl 'https://todo-backend-rustful.herokuapp.com/' -H 'Origin: http://www.todobackend.com' -H 'Accept-Encoding: gzip, deflate' -H 'Accept-Language: en-US,en;q=0.8' -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.125 Safari/537.36' -H 'Content-Type: application/json' -H 'Accept: text/plain, */*; q=0.01' -H 'Referer: http://www.todobackend.com/specs/index.html?https://todo-backend-rustful.herokuapp.com' -H 'Connection: keep-alive' --data-binary '{"title":"my todo"}' --compressed
{"title":"my todo","completed":false,"order":0,"url":"http://todo-backend-rustful.herokuapp.com/28"}
phodgson@applepie: ~/git  
⋙ curl http://todo-backend-rustful.herokuapp.com/28
{"title":"my todo","completed":false,"order":0,"url":"http://todo-backend-rustful.herokuapp.com/28"}

I think I will need your help to figure out what's your implementation sees as different between what Chrome is sending and what curl is sending.

@moredip
Copy link
Contributor

moredip commented Aug 9, 2015

Actually, when I subsequently ask Chrome to replay the same POST that created a null TODO it now succeeds! I'm now thinking perhaps your implementation has some sort of concurrency issue or race condition or something which shows up when the browser is firing off a large number of requests during the test run. Sound possible?

@Ogeon
Copy link

Ogeon commented Aug 9, 2015

Quite possible. The number of threads doesn't seem to be set, so there may be some congestion if the number of available CPU cores/threads are too low. This is a problem that is caused by the use of a thread pool in Hyper and it has been reported in hyperium/hyper#368.

I implemented the Todo example in a quite naive way, so I'm not surprised if there are some other problems lurking around, that I didn't think about.

@liamsi
Copy link

liamsi commented Aug 18, 2015

For me the implementation only works if (google) Chrome is started with a --disable-web-security flag (Version 45.0.2421.0 dev (64-bit)).
Without that I get an "XMLHttpRequest cannot load http://todo-backend-rustful.herokuapp.com/18. The request was redirected to 'https://todo-backend-rustful.herokuapp.com/18', which is disallowed for cross-origin requests that require preflight." error in the console.

@Ogeon
Copy link

Ogeon commented Aug 18, 2015

That's odd... There should not be a redirect, as far as I know, unless the actual list of allowed HTTP methods would count as it.

@Ogeon
Copy link

Ogeon commented Sep 20, 2015

Upgrading to Rustful 0.5 should make the congestion less of a problem. It has a mechanism that closes connections if the thread pool is about to become too occupied. It's not a solution, but a temporary measure until async IO is implemented.

@RoPP
Copy link
Contributor Author

RoPP commented Oct 6, 2015

Rustful 0.5 upgrade done! The heroku app is up to date too.

moredip added a commit that referenced this pull request Oct 13, 2015
Add a rust implementation.
@moredip moredip merged commit e7cc3a2 into TodoBackend:master Oct 13, 2015
@moredip
Copy link
Contributor

moredip commented Oct 13, 2015

Tests look good now. Thanks for the contribution!

@moredip moredip mentioned this pull request Oct 13, 2015
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.

4 participants