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

asynchttpserver form data/body broken with #13147 #13361

Closed
zedeus opened this issue Feb 8, 2020 · 10 comments
Closed

asynchttpserver form data/body broken with #13147 #13361

zedeus opened this issue Feb 8, 2020 · 10 comments

Comments

@zedeus
Copy link
Contributor

zedeus commented Feb 8, 2020

#13147 claims it does not break anything (and states it's an option), but it broke Jester's form data handling. Since there is a transition from body to bodyStream, this might have been intended.

import jester
routes:
  post "/":
    resp $request.params

nim c -d:useStdLib test.nim
curl -d 'q=foo' http://localhost:5000

With devel: {:}
Before the change: {"q": "foo"}

The Jester code responsible for getting the form data is not easily modifiable to support this change as it's non-async and relies on the old body field which is now useless.

@ghost
Copy link

ghost commented Feb 8, 2020

But #13146 wasn't merged (it's closed)?
Maybe you mean https://github.com/nim-lang/Nim/pull/13339/files#diff-3133183ae7c444b928e3e590f4b1d217R222 ?

@zedeus zedeus changed the title asynchttpserver form data/body broken with #13146 asynchttpserver form data/body broken with #13147 Feb 8, 2020
@zedeus
Copy link
Contributor Author

zedeus commented Feb 8, 2020

Sorry, relied on Github's inline preview menu and didn't notice the duplicate.

@mrhdias
Copy link
Contributor

mrhdias commented Feb 10, 2020

#13147 claims it does not break anything (and states it's an option), but it broke Jester's form data handling. Since there is a transition from body to bodyStream, this might have been intended.

import jester
routes:
  post "/":
    resp $request.params

nim c -d:useStdLib test.nim
curl -d 'q=foo' http://localhost:5000

With devel: {:}
Before the change: {"q": "foo"}

The Jester code responsible for getting the form data is not easily modifiable to support this change as it's non-async and relies on the old body field which is now useless.

Sorry, but, my first proposal didn't break anything, you can see the code here: . 4fdbf8f

But Dominik Picheta asked me if I could implement it with Future Streams. Yes, with Future Streams it break the things. But, now you can upload large files without exhaust all resources ;-)

@andreaferretti
Copy link
Collaborator

Rosencrantz was also broken recently, I am going to check if this is the reason

@mrhdias
Copy link
Contributor

mrhdias commented Feb 11, 2020

#13147 claims it does not break anything (and states it's an option), but it broke Jester's form data handling. Since there is a transition from body to bodyStream, this might have been intended.

import jester
routes:
  post "/":
    resp $request.params

nim c -d:useStdLib test.nim
curl -d 'q=foo' http://localhost:5000

With devel: {:}
Before the change: {"q": "foo"}

The Jester code responsible for getting the form data is not easily modifiable to support this change as it's non-async and relies on the old body field which is now useless.

@andreaferretti
Copy link
Collaborator

I confirm that #13147 also broke Rosencrantz. It passes all tests on bfe96e0, but crashes starting from 955465e, which is the merge commit.

I will have a look and see if I can fix them, but wasn't there a promise not break things randomly after 1.0?

@Araq
Copy link
Member

Araq commented Feb 11, 2020

I will have a look and see if I can fix them, but wasn't there a promise not break things randomly after 1.0?

Actually the promise is two-fold:

  1. Your code will continue to work with nim --useVersion:1.0.
  2. Your code has a >95% chance of continuing to work without the --useVersion emulation switch.

And yeah, it means we need to revert the PR that introduced the problem.

@andreaferretti
Copy link
Collaborator

Your code will continue to work with nim --useVersion:1.0.

Cool! I had missed that, and in fact Rosencrantz still works with that flag :-)

@mrhdias
Copy link
Contributor

mrhdias commented Feb 12, 2020

I will have a look and see if I can fix them, but wasn't there a promise not break things randomly after 1.0?

Actually the promise is two-fold:

1. Your code **will** continue to work with `nim --useVersion:1.0`.

2. Your code has a >95% chance of continuing to work without the --useVersion emulation switch.

And yeah, it means we need to revert the PR that introduced the problem.

Please, check the fix for this issue: #13394

@ringabout
Copy link
Member

ringabout commented Apr 8, 2022

#13147 was reverted by ec8a17c

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

5 participants