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

Revert broken asynchttpserver FutureStream additions. #13468

Merged
merged 1 commit into from
Mar 6, 2020

Conversation

dom96
Copy link
Contributor

@dom96 dom96 commented Feb 22, 2020

As discussed in #13394, these changes cannot work. Reverted via

git revert --no-commit 5bf571f061d53d35aab727f420afd9f415987723
git revert --no-commit abd660c407d00d0c4f2129ff11bfc69badda8ece
git revert --no-commit 955465e5f42b1353f69f3bd884908a7ef91ce13b
git commit

As discussed in #13394, these changes cannot work. Reverted via

```
git revert --no-commit 5bf571f
git revert --no-commit abd660c
git revert --no-commit 955465e
git commit
```
@Araq
Copy link
Member

Araq commented Feb 22, 2020

Pinq @mrhdias. As I said elsewhere, better than reverting this is fixing the feature so that it works well. It's an important feature.

@mrhdias
Copy link
Contributor

mrhdias commented Feb 23, 2020

Pinq @mrhdias. As I said elsewhere, better than reverting this is fixing the feature so that it works well. It's an important feature.

Thank you @Araq. The idea of using an iterator is not new.
#6576

I replace "let data = request.client.recv(readSize)" with "let data = waitFor request.client.recv(readSize)" to wait for the data.

So, reading the data is clear:

for data in req.bodyStream(8*1024):
  echo data

Iterator code

when (NimMajor, NimMinor) >= (1, 0):
  iterator bodyStream*(
    request: Request,
    chunkSize: int = 8*1024): 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 = waitFor request.client.recv(readSize)
      if data.len != readSize:
        raise newException(ValueError, "Error reading POST data from client.")
      yield data
      remainder -= readSize

@mrhdias
Copy link
Contributor

mrhdias commented Feb 23, 2020

Another solution forum (Thanks Hlaaftana): https://forum.nim-lang.org/t/5971

template forStream(req: Request, chunkSize: int, name, body: untyped): untyped =
  var remainder = req.contentLength
  while true:
    let `name` = await req.client.recv(min(remainder, chunkSize))
    body
    remainder -= len(`name`)
    if remainder == 0:
      break

req.forStream(8*1024, d):
  echo d

@dom96
Copy link
Contributor Author

dom96 commented Feb 23, 2020

It is better to fix this, but it's going to be much easier to review if we start fresh IMO. Can we merge this?

@dom96
Copy link
Contributor Author

dom96 commented Mar 6, 2020

Okay, I am merging this since there was no feedback in 12 days. Please ping me to review any new PRs on this.

@dom96 dom96 merged commit ec8a17c into devel Mar 6, 2020
@narimiran narimiran deleted the revert-asynchttpserver-changes branch March 9, 2020 20:18
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

Successfully merging this pull request may close these issues.

3 participants