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

http async replies, upgrade hook #324

Open
wants to merge 3 commits into
base: public
Choose a base branch
from

Conversation

dckc
Copy link
Contributor

@dckc dckc commented Jan 28, 2020

fixes #308

doesn't fix #313 but works around it to some extent

dckc added 3 commits January 27, 2020 19:30
If the headersComplete callback returns 'upgrade', return from the
server callback immediately rather than continuing to the
prepareResponse callback.
websockets.js doesn't check socket.write() to see how much data it can
send. The agoric pixel demo goes over 1k, so let's use 4k for now.
Copy link
Collaborator

@phoddie phoddie left a comment

Choose a reason for hiding this comment

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

I'm unsure about the goal of this change. It looks like it intentionally stalls-out the HTTP request after all the request headers have been received. How is that useful?

Copy link
Collaborator

@phoddie phoddie left a comment

Choose a reason for hiding this comment

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

By deign, the HTTP implementation does not use promises. We'll need to find another solution to the need this is addressing.

@dckc
Copy link
Contributor Author

dckc commented Aug 13, 2020

I'm unsure about the goal of this change. It looks like it intentionally stalls-out the HTTP request after all the request headers have been received. How is that useful?

As noted in #308, to upgrade an http server to websockets.

By deign, the HTTP implementation does not use promises.

I think I was also emulating the node http API, where replies can be computed asynchronously.

Can you elaborate? What design constraint conflicts with the use of promises?

I can see a number of outcomes here:

  • you decide that what I've requested here is OK (seems unlikely)
  • Agoric maintains its own http module that supports async and websockets upgrade
  • Agoric drops some of its requirements
    • In recent developments, we've reduced the scope of what we're using XS for; we don't use http nor sockets at all. But we might expand the scope again.

@phoddie
Copy link
Collaborator

phoddie commented Aug 13, 2020

I saw the note in #308 that the change is to support websocket upgrades. There is more going on which is not visible from the change. How does the proposed change support the websocket case you outline?

Regarding HTTP, the design constraint is specifically to not use Promises to eliminate unneeded overhead. Consequently, the PR cannot be accepted.

The potential solutions you enumerate above are: accept changes inconsistent with the implementation, fork, or change your goals. That overlooks my preferred solution - establish the requirements clearly, then design and implement a solution. Happy to work with you on that.

@dckc
Copy link
Contributor Author

dckc commented Aug 13, 2020

Oops; of course; I left the best outcome out. rephrase:

It's been a while since I knew exactly why I (thought I) needed this. I hope I can explain it better in due course, but other work seems to have higher priority these days; I can double-check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants