{"action":"created","issue":{"url":"https://api.github.com/repos/measurement-kit/measurement-kit/issues/929","repository_url":"https://api.github.com/repos/measurement-kit/measurement-kit","labels_url":"https://api.github.com/repos/measurement-kit/measurement-kit/issues/929/labels{/name}","comments_url":"https://api.github.com/repos/measurement-kit/measurement-kit/issues/929/comments","events_url":"https://api.github.com/repos/measurement-kit/measurement-kit/issues/929/events","html_url":"https://github.com/measurement-kit/measurement-kit/pull/929","id":190056660,"number":929,"title":"Simplify http parser","user":{"login":"bassosimone","id":337298,"avatar_url":"https://avatars.githubusercontent.com/u/337298?v=3","gravatar_id":"","url":"https://api.github.com/users/bassosimone","html_url":"https://github.com/bassosimone","followers_url":"https://api.github.com/users/bassosimone/followers","following_url":"https://api.github.com/users/bassosimone/following{/other_user}","gists_url":"https://api.github.com/users/bassosimone/gists{/gist_id}","starred_url":"https://api.github.com/users/bassosimone/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bassosimone/subscriptions","organizations_url":"https://api.github.com/users/bassosimone/orgs","repos_url":"https://api.github.com/users/bassosimone/repos","events_url":"https://api.github.com/users/bassosimone/events{/privacy}","received_events_url":"https://api.github.com/users/bassosimone/received_events","type":"User","site_admin":false},"labels":[{"id":217831593,"url":"https://api.github.com/repos/measurement-kit/measurement-kit/labels/cleanup","name":"cleanup","color":"fbca04","default":false},{"id":103088501,"url":"https://api.github.com/repos/measurement-kit/measurement-kit/labels/enhancement","name":"enhancement","color":"84b6eb","default":true},{"id":321404762,"url":"https://api.github.com/repos/measurement-kit/measurement-kit/labels/in%20progress","name":"in progress","color":"006b75","default":false},{"id":466236076,"url":"https://api.github.com/repos/measurement-kit/measurement-kit/labels/please%20preserve%20history","name":"please preserve history","color":"c2e0c6","default":false},{"id":473102883,"url":"https://api.github.com/repos/measurement-kit/measurement-kit/labels/refactoring","name":"refactoring","color":"d4c5f9","default":false}],"state":"closed","locked":false,"assignee":{"login":"bassosimone","id":337298,"avatar_url":"https://avatars.githubusercontent.com/u/337298?v=3","gravatar_id":"","url":"https://api.github.com/users/bassosimone","html_url":"https://github.com/bassosimone","followers_url":"https://api.github.com/users/bassosimone/followers","following_url":"https://api.github.com/users/bassosimone/following{/other_user}","gists_url":"https://api.github.com/users/bassosimone/gists{/gist_id}","starred_url":"https://api.github.com/users/bassosimone/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bassosimone/subscriptions","organizations_url":"https://api.github.com/users/bassosimone/orgs","repos_url":"https://api.github.com/users/bassosimone/repos","events_url":"https://api.github.com/users/bassosimone/events{/privacy}","received_events_url":"https://api.github.com/users/bassosimone/received_events","type":"User","site_admin":false},"assignees":[{"login":"bassosimone","id":337298,"avatar_url":"https://avatars.githubusercontent.com/u/337298?v=3","gravatar_id":"","url":"https://api.github.com/users/bassosimone","html_url":"https://github.com/bassosimone","followers_url":"https://api.github.com/users/bassosimone/followers","following_url":"https://api.github.com/users/bassosimone/following{/other_user}","gists_url":"https://api.github.com/users/bassosimone/gists{/gist_id}","starred_url":"https://api.github.com/users/bassosimone/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bassosimone/subscriptions","organizations_url":"https://api.github.com/users/bassosimone/orgs","repos_url":"https://api.github.com/users/bassosimone/repos","events_url":"https://api.github.com/users/bassosimone/events{/privacy}","received_events_url":"https://api.github.com/users/bassosimone/received_events","type":"User","site_admin":false}],"milestone":{"url":"https://api.github.com/repos/measurement-kit/measurement-kit/milestones/13","html_url":"https://github.com/measurement-kit/measurement-kit/milestone/13","labels_url":"https://api.github.com/repos/measurement-kit/measurement-kit/milestones/13/labels","id":1988331,"number":13,"title":"release-0.5.0","description":"","creator":{"login":"bassosimone","id":337298,"avatar_url":"https://avatars.githubusercontent.com/u/337298?v=3","gravatar_id":"","url":"https://api.github.com/users/bassosimone","html_url":"https://github.com/bassosimone","followers_url":"https://api.github.com/users/bassosimone/followers","following_url":"https://api.github.com/users/bassosimone/following{/other_user}","gists_url":"https://api.github.com/users/bassosimone/gists{/gist_id}","starred_url":"https://api.github.com/users/bassosimone/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bassosimone/subscriptions","organizations_url":"https://api.github.com/users/bassosimone/orgs","repos_url":"https://api.github.com/users/bassosimone/repos","events_url":"https://api.github.com/users/bassosimone/events{/privacy}","received_events_url":"https://api.github.com/users/bassosimone/received_events","type":"User","site_admin":false},"open_issues":37,"closed_issues":14,"state":"open","created_at":"2016-09-07T08:47:43Z","updated_at":"2017-01-21T15:25:01Z","due_on":"2017-03-15T07:00:00Z","closed_at":null},"comments":1,"created_at":"2016-11-17T14:07:58Z","updated_at":"2017-01-21T15:25:01Z","closed_at":"2017-01-21T15:25:01Z","pull_request":{"url":"https://api.github.com/repos/measurement-kit/measurement-kit/pulls/929","html_url":"https://github.com/measurement-kit/measurement-kit/pull/929","diff_url":"https://github.com/measurement-kit/measurement-kit/pull/929.diff","patch_url":"https://github.com/measurement-kit/measurement-kit/pull/929.patch"},"body":"*Note*: superseded by #1041. What follows is the original rationale. At bottom there is a slightly modified TODO list that highlights what to do before closing this PR.\r\n\r\n- - - \r\n\r\nThe code for reading incoming HTTP responses was quite messy and not simple to follow. In particular, it mixed callbacks and exceptions, which makes it more complex to reason about its correctness.\r\n\r\nThis pull request modified the implementation to avoid both callbacks and exception. Now, when you have incoming data, you pass it to `feed()`. This function now returns an `Error` and specifically it can return one of the following errors:\r\n\r\n1. *ParsingHeadersInProgressError*: meaning that the parser is still parsing response line and headers and more data is required to continue (this is similar to `EAGAIN`)\r\n\r\n2. *PausedAfterParsingHeadersError*: meaning that the parser has stopped after encountering end of headers, such that you can inspect response metadata, and can be restarted using `parse()` (which returns the same set of errors as `feed()`)\r\n\r\n3. *ParsingBodyInProgressError*: meaning that the parser is still parsing the body and more data is required to continue (this is similar to `EAGAIN`)\r\n\r\n4. *NoError*: meaning that the parser has successfully parsed a complete message\r\n\r\n5. Another, specific parser error (I have mapped all the errors relevant to us)\r\n\r\nOn EOF, you can call `eof()` that has similar return values.\r\n\r\nCompared to the previous implementation, also:\r\n\r\n1. the parser consumes all bytes that have been successfully parsed, allowing one to potentially know which was the first byte that failed the parser\r\n\r\n2. the parse is paused after a full message has been received and it is currently not possible to resume it, meaning that we currently do not support pipelining and, more importantly, if the server maliciously sends us two back to back, unexpected responses, we return parse error rather than presenting only the second response to the user (this is basically an issue of the implementation in 0.3.x).\r\n\r\nAll of this started as an attempt to address #741, but I quickly realized I was not satisfied with the implementation and I wanted to do more refactoring to make it more easily testable. However, the amount of work done is too much to land in stable (even though I did not change the API). At the same time, even though much more refactoring was possible, I avoided it in the interest of making incremental changes.\r\n\r\n- - - \r\n\r\nAs stated below, this pull request has been superseded by #1041. Yet, before closing it, there are some activities that we must perform the extract the good parts:\r\n\r\n- [ ] make sure we have no Valgrind related issues in #1041 \r\n- [ ] write more regress tests for parser aiming at increasing coverage and spotting more corner cases\r\n- [ ] add to #1041 note explaining that [stopping after headers would be complicated because of what seems to be a still open http-parser bug](https://github.com/nodejs/http-parser/issues/97)\r\n- [ ] specifically make sure we stop parsing after the first response [NOT NEEDED ANYMORE]\r\n- [ ] review HTTP requests we make and see whether we can make more of them\r\n- [ ] see if we can make a long web-connectivity test using Valgrind\r\n"},"comment":{"url":"https://api.github.com/repos/measurement-kit/measurement-kit/issues/comments/274268078","html_url":"https://github.com/measurement-kit/measurement-kit/pull/929#issuecomment-274268078","issue_url":"https://api.github.com/repos/measurement-kit/measurement-kit/issues/929","id":274268078,"user":{"login":"bassosimone","id":337298,"avatar_url":"https://avatars.githubusercontent.com/u/337298?v=3","gravatar_id":"","url":"https://api.github.com/users/bassosimone","html_url":"https://github.com/bassosimone","followers_url":"https://api.github.com/users/bassosimone/followers","following_url":"https://api.github.com/users/bassosimone/following{/other_user}","gists_url":"https://api.github.com/users/bassosimone/gists{/gist_id}","starred_url":"https://api.github.com/users/bassosimone/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bassosimone/subscriptions","organizations_url":"https://api.github.com/users/bassosimone/orgs","repos_url":"https://api.github.com/users/bassosimone/repos","events_url":"https://api.github.com/users/bassosimone/events{/privacy}","received_events_url":"https://api.github.com/users/bassosimone/received_events","type":"User","site_admin":false},"created_at":"2017-01-21T15:25:01Z","updated_at":"2017-01-21T15:25:01Z","body":"To conclude this pull request. In addition to the refactoring of how we read, which was done better in #1041, this pull request contained two other interesting developments:\r\n\r\n1. a complete enumeration of parser error code, now submitted as #1095 \r\n\r\n2. code to prevent back to back requests to confuse the parser (added as a requirement to consider #1041 and ready for review again)\r\n\r\nGiven that everything interesting was either extracted or implemented better, we can now close."}}
{"actor":{"display_login":"bassosimone"}}