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.

Sign up for GitHub

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

Transfer-Encoding:chunked tests #16678

Merged
merged 16 commits into from
Jan 14, 2021
Merged

Transfer-Encoding:chunked tests #16678

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
bb12d43
Add tests and fix extra newlines in body
vabresto Jan 11, 2021
09ad8ea
Fixes per comments
vabresto Jan 11, 2021
fd61db3
Slight rephrase per comments
vabresto Jan 11, 2021
c36f855
Improvements per comments
vabresto Jan 12, 2021
1fd913a
Add getSocket to reduce test flakiness per comment
vabresto Jan 12, 2021
c850be5
Remove unused lines from header
vabresto Jan 12, 2021
057c88d
Add doc comment to getSocket per comment
vabresto Jan 13, 2021
d5035ba
Apply witchcraft to replace `discard Future`
vabresto Jan 13, 2021
08bb764
Return HTTP 400 on bad encoding in request
vabresto Jan 13, 2021
df08ce2
Fix runnable example for getSocket
vabresto Jan 13, 2021
3ca6cf8
Fix import to fix runnable examples
vabresto Jan 13, 2021
27f9714
Even more imports for the example
vabresto Jan 13, 2021
03af833
Better self documenting runnable example
vabresto Jan 13, 2021
a7a916a
Add missing import
vabresto Jan 14, 2021
b2433a3
Import from module with correct signature
vabresto Jan 14, 2021
f3b2aad
Resolve port type mismatch
vabresto Jan 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions lib/pure/asynchttpserver.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ runnableExamples:

import asyncnet, asyncdispatch, parseutils, uri, strutils
import httpcore
import std/private/since

export httpcore except parseHeader

Expand Down Expand Up @@ -71,6 +72,9 @@ type
maxBody: int ## The maximum content-length that will be read for the body.
maxFDs: int

func getSocket*(a: AsyncHttpServer): AsyncSocket {.since: (1, 5, 1).} =
a.socket

proc newAsyncHttpServer*(reuseAddr = true, reusePort = false,
maxBody = 8388608): AsyncHttpServer =
## Creates a new ``AsyncHttpServer`` instance.
Expand Down Expand Up @@ -300,9 +304,14 @@ proc processRequest(
break

# Read bytesToRead and add to body
# Note we add +2 because the line must be terminated by \r\n
let chunk = await client.recv(bytesToRead + 2)
request.body = request.body & chunk
let chunk = await client.recv(bytesToRead)
request.body.add(chunk)
# Skip \r\n (chunk terminating bytes per spec)
when compileOption("assertions"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this, assert is already removed by the compiler when this option is not specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was told in a different comment (or maybe IRC) that doing something like this would save an extra string allocation. Additionally, if one compiles with asserts off, will

        let separator = await client.recv(2)
        assert separator == "\r\n"

cause an Unused identifier warning?

That said, I don't mind removing it as it definitely looks cleaner without it.

let separator = await client.recv(2)
assert separator == "\r\n"
Copy link
Member

@timotheecour timotheecour Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this fail in practice (eg client that violates spec)? if so, it's an external condition (not internal logic), so raising exception should be used instead

silently skipping the last 2 bytes can be really bad if they're not \r\n.

I doubt checking this condition has any real performance impact compared to rest of surrounding code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it is external input, so sure, it can trigger the assert, but it would be a malformed request according to (my understanding of) the spec.

If the bytes are never received, the await should never finish, so I don't think an exception would be thrown.
If the bytes are received, but are not "\r\n", they simply get discarded. I also don't think this is exception worthy, especially if exceptions are explicitly disabled.

I'm not sure (and have no strong feelings) about what the "right" thing to do if the last two bytes are not "\r\n", but I would err towards saying that it is a malformed request (maybe return 400?) and is not a server exception.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do other languages do?

Copy link
Contributor Author

@vabresto vabresto Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what Python does: https://github.com/python/cpython/blob/63298930fb531ba2bb4f23bc3b915dbf1e17e9e1/Lib/http/client.py#L558

        if not chunk_left: # Can be 0 or None
            if chunk_left is not None:
                # We are at the end of chunk, discard chunk end
                self._safe_read(2)  # toss the CRLF at the end of the chunk

where self._safe_read is defined as

    def _safe_read(self, amt):
        """Read the number of bytes requested.
        This function should be used when <amt> bytes "should" be present for
        reading. If the bytes are truly not available (due to EOF), then the
        IncompleteRead exception can be used to detect the problem.
        """
        data = self.fp.read(amt)
        if len(data) < amt:
            raise IncompleteRead(data, amt-len(data))
        return data

Copy link
Member

@timotheecour timotheecour Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok we can deal with it in future PR's then, keep the code as is (with when compileOption("assertions"): ...

maybe with a comment:

# xxx figure out whether to raise, assert or doAssert

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a TODO just do it properly: raise a ValueError.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with ValueError and that was my initial wish too in #16678 (comment)

else:
discard await client.recv(2)

inc sizeOrData
elif request.reqMethod == HttpPost:
Expand Down
75 changes: 75 additions & 0 deletions tests/stdlib/tasynchttpserver_transferencoding.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import httpclient, asynchttpserver, asyncdispatch, asyncfutures
import net

import std/asyncnet
import std/nativesockets

const postBegin = """
POST / HTTP/1.1
Transfer-Encoding:chunked

"""

template genTest(input, expected) =
var sanity = false
proc handler(request: Request) {.async.} =
doAssert(request.body == expected)
doAssert(request.headers.hasKey("Transfer-Encoding"))
doAssert(not request.headers.hasKey("Content-Length"))
sanity = true
await request.respond(Http200, "Good")

let server = newAsyncHttpServer()
discard server.serve(Port(0), handler)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(previous comment got lost)
discarding a future seems buggy (eg refs #11912)

note that #16695 doesn't have this issue (see its code with await server.serve(Port(5555), cb)), can you make something like it work? if not, add a comment:

# xxx figure out whether there's a better way than discarding a Future here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I spent some time and dug through some docs, and with a bit of intuition and a lot of luck, I was able to replace this by using callSoon. I have no clue if that solution is any better than using discard, but I couldn't replicate what #16695 was doing.

Please advise.

Copy link
Member

@timotheecour timotheecour Jan 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: I don't know if callSoon is the right thing here; runSleepLoop seems more complicated than it should, there may be a better, but I don't know which, so please add a
# xxx check if there is a better way so we can move forward with this PR

Copy link
Contributor

@dom96 dom96 Jan 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely not the right thing here. All you need to do is use asyncCheck on the code that calls asynchttpserver's serve. I think you've made the code harder to work with by creating that genTest template which is not necessary, turn it into an async proc and then waitFor that async proc.

let port = getLocalAddr(server.getSocket.getFd, AF_INET)[1]
let data = postBegin & input
var socket = newSocket()
socket.connect("127.0.0.1", port)
socket.send(data)
waitFor sleepAsync(10)
socket.close()
server.close()

# Verify we ran the handler and its asserts
doAssert(sanity)

block:
const expected = "hello=world"
const input = ("b\r\n" &
"hello=world\r\n" &
"0\r\n" &
"\r\n")
genTest(input, expected)
block:
const expected = "hello encoding"
const input = ("e\r\n" &
"hello encoding\r\n" &
"0\r\n" &
"\r\n")
genTest(input, expected)
block:
# https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding
const expected = "MozillaDeveloperNetwork"
const input = ("7\r\n" &
"Mozilla\r\n" &
"9\r\n" &
"Developer\r\n" &
"7\r\n" &
"Network\r\n" &
"0\r\n" &
"\r\n")
genTest(input, expected)
block:
# https://en.wikipedia.org/wiki/Chunked_transfer_encoding#Example
const expected = "Wikipedia in \r\n\r\nchunks."
const input = ("4\r\n" &
"Wiki\r\n" &
"6\r\n" &
"pedia \r\n" &
"E\r\n" &
"in \r\n" &
"\r\n" &
"chunks.\r\n" &
"0\r\n" &
"\r\n")
genTest(input, expected)