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

can't serve static files more than 10 MB using jester #241

Open
ringabout opened this issue Mar 22, 2020 · 11 comments
Open

can't serve static files more than 10 MB using jester #241

ringabout opened this issue Mar 22, 2020 · 11 comments

Comments

@ringabout
Copy link

Test Code in:
https://github.com/xflywind/test_jester

Got Error: ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_LENGTH

Because jester sends multiple content length.

# in asynchttpserver.nim
# https://github.com/nim-lang/Nim/blob/b6924383df63c91f0ad6baf63d0b1aa84f9329b7/lib/pure/asynchttpserver.nim#L102
proc respond*(req: Request, code: HttpCode, content: string,
              headers: HttpHeaders = nil): Future[void] =
  if headers != nil:
    msg.addHeaders(headers)
  msg.add("Content-Length: ")
  # this particular way saves allocations:
  msg.add content.len
  msg.add "\c\L\c\L"
  msg.add(content)
  result = req.client.send(msg)
# in jester
# https://github.com/dom96/jester/blob/d8a03aa4c681bc8514bb7bbf4953d380d86f5bd6/jester.nim#L202
let headers = @({
  "Content-Type": mimetype,
  "Content-Length": $fileSize
})
await req.statusContent(Http200, "", some(headers))

Relative codes:

"Content-Length": $fileSize

https://github.com/dom96/httpbeast/blob/469fbe2eb029d0dfcb8e6b6ec0a37c52b7fc2313/src/httpbeast.nim#L332

https://github.com/nim-lang/Nim/blob/b6924383df63c91f0ad6baf63d0b1aa84f9329b7/lib/pure/asynchttpserver.nim#L102

@JohnAD
Copy link

JohnAD commented Apr 2, 2020

I'm getting the same problem.

As a temporary work-around, I'm sending the content directly. Granted, that eats up some serious RAM. Keeping that GC busy :).

Stuff like:

  get "/download/xyz.pdf":
    let blob = readFile("public/download/xyz.pdf")
    resp Http200, blob, "application/pdf"

@ringabout
Copy link
Author

It works.But we will rewrite async http server from scratch instead. 😄

@ringabout
Copy link
Author

other workarounds by modifying asynchttpserver.So does httpbeast.

# in asynchttpserver.nim
# https://github.com/nim-lang/Nim/blob/b6924383df63c91f0ad6baf63d0b1aa84f9329b7/lib/pure/asynchttpserver.nim#L102
proc respond*(req: Request, code: HttpCode, content: string,
              headers: HttpHeaders = nil): Future[void] =
  if headers != nil:
    msg.addHeaders(headers)
  # change this
  if not headers.hasKey("Content-Length"):
    msg.add("Content-Length: ")
    # this particular way saves allocations:
    msg.add content.len
  msg.add "\c\L\c\L"
  msg.add(content)
  result = req.client.send(msg)

@JohnAD
Copy link

JohnAD commented Apr 2, 2020

@xflywind The change you are suggesting for asynchttpserver seems very reasonable and useful for the standard library. Are you going to do a PR to Nim for that?

@ringabout
Copy link
Author

I'll try.Maybe need a better solution instead.😄

@ringabout
Copy link
Author

Now it works with asynchttpserver.

@JohnAD
Copy link

JohnAD commented Apr 5, 2020

@xflywind the pull request was accepted just in time and made into the 1.2 release of Nim!

@jasonprogrammer
Copy link

I use Jester with threads enabled, and am experiencing this file download issue with httpbeast (a Content-Length: 0 is indeed being sent prior to a Content-Length header indicating the size of the file).

I was able to modify the send method in httpbeast.nim to optionally send the Content-Length header (see the includeContentLen variable):

proc send*(req: Request, code: HttpCode, body: string, headers="",
           includeContentLen=true) =
  ## Responds with the specified HttpCode and body.
  ##
  ## **Warning:** This can only be called once in the OnRequest callback.

  if req.client notin req.selector:
    return

  # TODO: Reduce the amount of `getData` accesses.
  template getData: var Data = req.selector.getData(req.client)
  assert getData.headersFinished, "Selector not ready to send."

  let otherHeaders = if likely(headers.len == 0): "" else: "\c\L" & headers
  var text = "HTTP/1.1 $#\c\L" % [$code]

  if includeContentLen:
      text &= "Content-Length: $#\c\L" % [$body.len]

  text &= ("Server: $#\c\LDate: $#$#\c\L\c\L$#") % [
    serverInfo, serverDate, otherHeaders, body
  ]

  getData.sendQueue.add(text)
  req.selector.updateHandle(req.client, {Event.Read, Event.Write})

and then made this modification on the Jester side, to set includeContentLen based on the existence of the Content-Length header (the headers are passed as tuples, so I had to iterate over them):

proc send(
  request: Request, code: HttpCode, headers: Option[RawHeaders], body: string
): Future[void] =
  when useHttpBeast:
    let h =
      if headers.isNone: ""
      else: headers.get().createHeaders

    var includeContentLen = true
    for header in headers.get():
      if header[0] == "Content-Length":
        includeContentLen = false
        break
    request.getNativeReq.send(code, body, h, includeContentLen)
    var fut = newFuture[void]()
    complete(fut)
    return fut
  else:
    return request.getNativeReq.respond(
      code, body, newHttpHeaders(headers.get(@({:})))
    )

This feels hacky, but the headers are sent to httpbeast as a string (so I can't just do a hasKey check on the HttpBeast side). Thoughts? Should I create a PR, or should this be addressed some other way?

@dom96
Copy link
Owner

dom96 commented Jul 4, 2020

Thoughts? Should I create a PR, or should this be addressed some other way?

You should create an overload that sends only headers in httpbeast or actually just use unsafeSend in jester to send both manually (https://github.com/dom96/httpbeast/blob/master/src/httpbeast.nim#L307)

@jasonprogrammer
Copy link

jasonprogrammer commented Jul 4, 2020

Good suggestion! How's this? #253. It works for me both for async and with threads. Update: It doesn't work when compiling with -d:useStdLib.

This ends up omitting the Server and Date headers, so the download headers look like this:

HTTP/1.1 200 OK
Content-Type: application/zip
Content-Length: 10439765

@ringabout
Copy link
Author

ringabout commented Oct 2, 2020

Now, httpx(a fork of httpbeast) has partly solved streaming file support.

I has overloaded send proc. It works fine in linux. Someone can copy them back.

https://github.com/xflywind/httpx/blob/82f5e8baff3f3f0c757f5b04599616f15c936049/src/httpx.nim#L401

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

No branches or pull requests

4 participants