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

Jump to bottom

Use async to fix asynchttpserver #236

Merged
merged 2 commits into from
Feb 12, 2020
Merged

Use async to fix asynchttpserver #236

merged 2 commits into from
Feb 12, 2020

Conversation

zedeus
Copy link
Contributor

@zedeus zedeus commented Jan 28, 2020

This fixes the "Bad file descriptor" error when running asynchttpserver behind an nginx instance. There might be some more serious underlying issue causing this, but for now I see it as Jester using asynchttpserver incorrectly which is fixed in this pr. Tested with both asynchttpserver and httpbeast.

@zedeus zedeus mentioned this pull request Jan 28, 2020
Closed
dom96
dom96 requested changes Jan 28, 2020
View reviewed changes
jester.nim Outdated Show resolved Hide resolved
@zedeus
Copy link
Contributor Author

zedeus commented Jan 29, 2020 •
edited
Loading

Alright, got rid of {.async.} but kept the futures since that's needed to return the final send future to asynchttpserver so it can be awaited. I'm not sure if this solves the problem of heap allocations, let me know what you think.

@zedeus zedeus force-pushed the asyncify branch 3 times, most recently from 3481594 to ea583e0 Compare January 29, 2020 02:21
@mrhdias
Copy link

mrhdias commented Feb 12, 2020

This fixes the "Bad file descriptor" error when running asynchttpserver behind an nginx instance. There might be some more serious underlying issue causing this, but for now I see it as Jester using asynchttpserver incorrectly which is fixed in this pr. Tested with both asynchttpserver and httpbeast.

Please, check the fix for this issue: nim-lang/Nim#13394

@zedeus
Copy link
Contributor Author

zedeus commented Feb 12, 2020 •
edited
Loading

@mrhdias Not sure why you're replying to this pr, it has nothing to do with your changes

dom96
dom96 approved these changes Feb 12, 2020
View reviewed changes
Copy link
Owner

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Nice, looks good. One small thing is I want an assert, but I can add that.

jester.nim
Comment on lines -401 to -403
let future = newFuture[void]()
complete(future)
return future
Copy link
Owner

Choose a reason for hiding this comment

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

Cool, so this was simply moved to send.

@dom96 dom96 merged commit a65dbf7 into dom96:master Feb 12, 2020
@dom96 dom96 mentioned this pull request Jun 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dom96 dom96 dom96 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zedeus @mrhdias @dom96