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

Show file tree
Hide file tree
Changes from all 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
3 changes: 2 additions & 1 deletion changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@

## Library changes

- `asynchttpserver` now the request body is a FutureStream.
- `asynchttpserver` added an iterator that allows the request body to be read in
chunks of data when new server "stream" option is set to true.
- `asyncdispatch.drain` now properly takes into account `selector.hasPendingOperations`
and only returns once all pending async operations are guaranteed to have completed.
- `asyncdispatch.drain` now consistently uses the passed timeout value for all
Expand Down
116 changes: 73 additions & 43 deletions lib/pure/asynchttpserver.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@
## import asynchttpserver, asyncdispatch
## import strutils, strformat
##
## const stream = true # for test purposes switch from true to false
##
## 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>
## <head><meta charset="utf-8"/></head>
## <body>
## <form action="/" method="post" enctype="multipart/form-data">
## File: <input type="file" name="testfile" accept="text/*"><br />
Expand All @@ -65,17 +65,23 @@
## 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
## if stream:
## # Read 8*1024 bytes at a time
## # optional chunkSize parameter. The default is 8*1024
## for length, data in req.bodyStream(8*1024):
## 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
## else:
## # read 8*1024 bytes at a time
## while (let data = await req.bodyStream.read(); data[0]):
## bodyLength += data[1].len
## bodyLength += req.body.len
## await req.respond(Http200, htmlpage(contentLength, bodyLength))
##
## let server = newAsyncHttpServer(maxBody = 10485760) # 10 MB
## let server = newAsyncHttpServer(maxBody = 10485760, stream = stream) # 10 MB
## waitFor server.serve(Port(8080), cb)

import tables, asyncnet, asyncdispatch, parseutils, uri, strutils
Expand All @@ -93,9 +99,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 +107,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 +128,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 +214,30 @@ 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):
iterator bodyStream*(
request: Request,
chunkSize: int = 8*1024): (int, Future[string]) =
## The chunkSize parameter is optional and default value is 8*1024 bytes.
##
## This iterator return a tuple with the length of the data that was read
## and a future.
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 +251,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 +350,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