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

Unblock server on multiple HTTP requests #3

Merged
merged 7 commits into from
Dec 6, 2018

Conversation

kevinkassimo
Copy link
Contributor

@kevinkassimo kevinkassimo commented Dec 4, 2018

Closes #1 .
Resolve the immediate cause for blocking, by making new conn accept and readRequest of same level and async, with Promise.race. Now http_test.ts should work properly.

(Currently there will be warnings of Promise resolved after resolved. The warning would go away after denoland/deno#1273)

Socket is closed for now once responding. Will need to change this behavior later. Sockets are left open now until EOF.

I'm not quite sure if this fix is proper, and whether I should keep serveConn (since it is exported)
I will work more on http based on feedback for this change.

@CLAassistant
Copy link

CLAassistant commented Dec 4, 2018

CLA assistant check
All committers have signed the CLA.

@kevinkassimo kevinkassimo changed the title Unblock multiple HTTP requests Unblock server on multiple HTTP requests Dec 4, 2018
Copy link
Contributor

@zhmushan zhmushan left a comment

Choose a reason for hiding this comment

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

terminal print out:

Promise warning: Promise resolved after resolved
Promise warning: Promise resolved after resolved
Promise warning: Promise resolved after resolved

@kevinkassimo
Copy link
Contributor Author

@zhmushan It’s a deno issue, see denoland/deno#1273

@zhmushan
Copy link
Contributor

zhmushan commented Dec 4, 2018

@zhmushan It’s a deno issue, see denoland/deno#1273

Sorry, I just saw this issue.😭

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Nice work - I'm glad you got it working :)
It's unfortunate that this logic is so complex now.. Isn't there some simpler way this can be done? Ideally without using Promise.race ?

http.ts Outdated Show resolved Hide resolved
@kevinkassimo
Copy link
Contributor Author

Kinda bit stuck with implementing a simpler logic. (After getting rid of Promise.race I would really hope that the deferred and new array each time could also go away...)

http.ts Outdated Show resolved Hide resolved
http.ts Show resolved Hide resolved
http.ts Show resolved Hide resolved
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 1bf555a into denoland:master Dec 6, 2018
@ry ry mentioned this pull request Jan 19, 2019
ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019
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.

http_test.ts
4 participants