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

Fix to asynchttpserver form data/body broken with #13147 #13394

Merged
merged 6 commits into from
Feb 14, 2020
Merged

Fix to asynchttpserver form data/body broken with #13147 #13394

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3bacfd1
Fix to asynchttpserver form data/body broken with #13147
mrhdias Feb 12, 2020
896ca41
Fixed a small text error!
mrhdias Feb 12, 2020
b1930d6
New implementation that use a interator instance of future streams
mrhdias Feb 14, 2020
e550536
Two examples have been condensed into one.
mrhdias Feb 14, 2020
d68cbe3
Fix a small error!
mrhdias Feb 14, 2020
4414cd1
asynchttpserver now can handle chunks of data.
mrhdias Feb 14, 2020
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
146 changes: 106 additions & 40 deletions lib/pure/asynchttpserver.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@
## respond with a page with the actual and expected body length after
## submitting a file.
##
## With option stream set to true:
## -------------------------------
##
## .. code-block::nim
## import asynchttpserver, asyncdispatch
## import strutils, strformat
Expand Down Expand Up @@ -65,14 +68,55 @@
## bodyLength = 0
## if req.reqMethod == HttpPost:
## contentLength = req.headers["Content-length"].parseInt
## if contentLength < 8*1024: # the default chunkSize
## # read the request body at once
## let body = await req.bodyStream.readAll();
## bodyLength = body.len
## else:
## # read 8*1024 bytes at a time
## while (let data = await req.bodyStream.read(); data[0]):
## bodyLength += data[1].len
## # Read 8*1024 bytes at a time
## for length, data in req.bodyStream():
## let content = await data
## if length == content.len:
## bodyLength += content.len
## else:
## # Handle exception
## await req.respond(Http400,
## "Bad Request. Data read has a different length than the expected.")
## return
## await req.respond(Http200, htmlpage(contentLength, bodyLength))
##
## let server = newAsyncHttpServer(maxBody = 10485760, stream = true) # 10 MB
## waitFor server.serve(Port(8080), cb)
##
## With default option stream (false is the default):
## --------------------------------------------------
##
## .. code-block::nim
## import asynchttpserver, asyncdispatch
## import strutils, strformat
##
## proc htmlpage(contentLength, bodyLength: int): string =
## return &"""
## <!Doctype html>
## <html lang="en">
mrhdias marked this conversation as resolved.
Show resolved Hide resolved
## <head>
## <meta charset="utf-8"/>
## </head>
## <body>
## <form action="/" method="post" enctype="multipart/form-data">
## File: <input type="file" name="testfile" accept="text/*"><br />
## <input style="margin:10px 0;" type="submit">
## </form><br />
## Expected Body Length: {contentLength} bytes<br />
## Actual Body Length: {bodyLength} bytes
## </body>
## </html>
## """
##
## proc cb(req: Request) {.async.} =
## var
## contentLength = 0
## bodyLength = 0
## if req.reqMethod == HttpPost:
## contentLength = req.headers["Content-length"].parseInt
## # Read all body at once
## let body = req.body
## bodyLength = body.len
## await req.respond(Http200, htmlpage(contentLength, bodyLength))
##
## let server = newAsyncHttpServer(maxBody = 10485760) # 10 MB
Expand All @@ -93,9 +137,6 @@ const
maxLine = 8*1024

when (NimMajor, NimMinor) >= (1, 1):
const
chunkSize = 8*1024 ## This seems perfectly reasonable for default chunkSize.

type
Request* = object
client*: AsyncSocket # TODO: Separate this into a Response object?
Expand All @@ -104,8 +145,16 @@ when (NimMajor, NimMinor) >= (1, 1):
protocol*: tuple[orig: string, major, minor: int]
url*: Uri
hostname*: string ## The hostname of the client that made the request.
body*: string # For future removal
bodyStream*: FutureStream[string]
body*: string
contentLength*: int

type
AsyncHttpServer* = ref object
socket: AsyncSocket
reuseAddr: bool
reusePort: bool
maxBody: int ## The maximum content-length that will be read for the body.
stream: bool ## By default (stream = false), the body of the request is read immediately
else:
type
Request* = object
Expand All @@ -117,20 +166,30 @@ else:
hostname*: string ## The hostname of the client that made the request.
body*: string

type
AsyncHttpServer* = ref object
socket: AsyncSocket
reuseAddr: bool
reusePort: bool
maxBody: int ## The maximum content-length that will be read for the body.
type
AsyncHttpServer* = ref object
socket: AsyncSocket
reuseAddr: bool
reusePort: bool
maxBody: int ## The maximum content-length that will be read for the body.

proc newAsyncHttpServer*(reuseAddr = true, reusePort = false,
when (NimMajor, NimMinor) >= (1, 1):
proc newAsyncHttpServer*(reuseAddr = true, reusePort = false,
maxBody = 8388608, stream = false): AsyncHttpServer =
## Creates a new ``AsyncHttpServer`` instance.
new result
result.reuseAddr = reuseAddr
result.reusePort = reusePort
result.maxBody = maxBody
result.stream = stream
else:
proc newAsyncHttpServer*(reuseAddr = true, reusePort = false,
maxBody = 8388608): AsyncHttpServer =
## Creates a new ``AsyncHttpServer`` instance.
new result
result.reuseAddr = reuseAddr
result.reusePort = reusePort
result.maxBody = maxBody
## Creates a new ``AsyncHttpServer`` instance.
new result
result.reuseAddr = reuseAddr
result.reusePort = reusePort
result.maxBody = maxBody

proc addHeaders(msg: var string, headers: HttpHeaders) =
for k, v in headers:
Expand Down Expand Up @@ -193,13 +252,28 @@ proc parseProtocol(protocol: string): tuple[orig: string, major, minor: int] =
proc sendStatus(client: AsyncSocket, status: string): Future[void] =
client.send("HTTP/1.1 " & status & "\c\L\c\L")

when (NimMajor, NimMinor) >= (1, 1):
## chunkSize is optional and default value is 8*1024 bytes.
iterator bodyStream*(
request: Request,
chunkSize: int = 8*1024): (int, Future[string]) =

var remainder = request.contentLength
while remainder > 0:
let readSize = min(remainder, chunkSize)
let data = request.client.recv(readSize)
if data.failed:
raise newException(ValueError, "Error reading POST data from client.")
yield (readSize, data)
remainder -= readSize
Comment on lines +225 to +232
Copy link
Contributor

Choose a reason for hiding this comment

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

This iterator is quite broken and makes me wonder whether you've tested this at all:

  • recv reads up to readSize, it doesn't guarantee you'll get that much data, and a lot of the time you won't.

  • you're checking whether the data future failed, without awaiting it. This code path will almost never trigger.

Copy link
Contributor Author

@mrhdias mrhdias Feb 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You can test with the example and you will see that it works. Try uploading a 900 MB file! Please give me a solution or a way to implement this.

Copy link
Contributor Author

@mrhdias mrhdias Feb 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This iterator is quite broken and makes me wonder whether you've tested this at all:

* `recv` reads **up to** `readSize`, it doesn't guarantee you'll get that much data, and a lot of the time you won't.

* you're checking whether the `data` future failed, without `await`ing it. This code path will almost never trigger.

From the nim documentation (https://nim-lang.org/0.16.0/asyncdispatch.html)

var future = sock.recv(100)
yield future
if future.failed:
  # Handle exception

I can't use the await in the iterators!
#3885
Async iterators #6576
#6576


proc processRequest(
server: AsyncHttpServer,
req: FutureVar[Request],
client: AsyncSocket,
address: string,
lineFut: FutureVar[string],
callback: proc (request: Request): Future[void] {.closure, gcsafe.},
callback: proc (request: Request): Future[void] {.closure, gcsafe.}
): Future[bool] {.async.} =

# Alias `request` to `req.mget()` so we don't have to write `mget` everywhere.
Expand All @@ -213,13 +287,9 @@ proc processRequest(
request.hostname.shallowCopy(address)
assert client != nil
request.client = client
request.body = ""
when (NimMajor, NimMinor) >= (1, 1):
request.bodyStream = newFutureStream[string]()
# To uncomment in the future after compatibility issues
# with third parties are solved
# else:
# request.body = ""
request.body = "" # Temporary fix for future removal
request.contentLength = 0

# We should skip at least one empty line before the request
# https://tools.ietf.org/html/rfc7230#section-3.5
Expand Down Expand Up @@ -316,16 +386,12 @@ proc processRequest(
return false

when (NimMajor, NimMinor) >= (1, 1):
var remainder = contentLength
while remainder > 0:
let readSize = min(remainder, chunkSize)
let data = await client.recv(read_size)
if data.len != read_size:
request.contentLength = contentLength
if not server.stream:
request.body = await client.recv(contentLength)
if request.body.len != contentLength:
await request.respond(Http400, "Bad Request. Content-Length does not match actual.")
return true
await request.bodyStream.write(data)
remainder -= data.len
request.bodyStream.complete()
else:
request.body = await client.recv(contentLength)
if request.body.len != contentLength:
Expand Down