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

Deno.serve issues #15551

Closed
4 of 14 tasks
Tracked by #17146
lucacasonato opened this issue Aug 23, 2022 · 8 comments
Closed
4 of 14 tasks
Tracked by #17146

Deno.serve issues #15551

lucacasonato opened this issue Aug 23, 2022 · 8 comments
Labels
bug Something isn't working correctly

Comments

@lucacasonato
Copy link
Member

lucacasonato commented Aug 23, 2022

Quick list of things I think we need to address with the new --unstable Deno.serve API.

  • Rename the API. Deno.serve does not clearly communicate the relation to HTTP. Strawman: Deno.listenAndServeHttp. Anyone else have other ideas?
  • Change signature to (handler: (Request, ConnInfo) => Response, opts?: ListenAndServeHttpOptions) => Promise<void> (move handler out of options bag).
  • Rename cert and key options to certFile and keyFile.
  • Add certData and keyData options that don't require reading these files from disk.
  • Errors returned from server.serve() should not be swallowed with a .catch(console.error);.
  • Errors occuring in server.serve should clean up server resources
  • Each server should have it's own date interval, otherwise each invocation to the API creates a different number of resources, which is confusing to users when using the test resource sanitizer.
  • flash Date header time drift #15574
  • op_flash_make_request fast ops for multiple servers #15572
  • Remove all of the unwraps when handing values from JS.
  • Flash does not handle socket write failures at all. If a write fails, or only writes partial data, we completely ignore this and just go onto the next write...
  • The safety comments very often don't explain why something is safe. They just say "it's safe".
  • Flash can not handle arbitrary methods.
  • flash should set sendfile count to size returned by stat #15573
@lucacasonato lucacasonato changed the title Deno.serve issue checklist Deno.serve issues Aug 23, 2022
@kitsonk
Copy link
Contributor

kitsonk commented Aug 23, 2022

These are the 3 things that are keeping me from making flash enabled by default for oak:

  • There currently isn't a way to obtain the remote connection IP address, meaning ctx.request.ip and ctx.request.ips do not work properly.
  • There appears to be an issue with streaming large files with flash.
  • Some of the exception handling with flash isn't as flexible as the "native" implementation, meaning that certain errors result in a hard coded 500 internal server error as well as "invalid" responses result in a server hang.

@kt3k
Copy link
Member

kt3k commented Aug 23, 2022

@lucacasonato

Rename cert and key options to certFile and keyFile.
Add certData and keyData options that don't require reading these files from disk.

Should we support both certData/keyData and certFile/keyFile?

We've already deprecated certFile/keyFile options in ListenOptions #13740, removed certFile in StartTlsOption when stabilizing (#12581). certFile in ConnectTlsOption is also deprecated #12219

@billywhizz
Copy link
Contributor

it would be nice to have access to the lower level socket options in some way also. failing that, some configurable options in api to set tcp keepAlive, noDelay, reusePort, reuseAddress etc. would be good. currently, i have no way to spin up multiple instances and have the kernel do load balancing across them easily, which reusePort would enable.

@littledivy
Copy link
Member

@billywhizz There is Deno.upgradeHttp(request) to get the tcp stream (but not for the tcp listener). I think we should enable reuse port by default.

@lucacasonato
Copy link
Member Author

lucacasonato commented Aug 24, 2022

Should we support both certData/keyData and certFile/keyFile?

@kt3k We can also remove the certFile/keyFile options. I don't really mind either way

@st0le
Copy link

st0le commented Aug 26, 2022

Minor thing.

When using npm:imports, I noticed 1.25 prints an error asking the user to run with --unstable flag.

The same doesn't happen for Deno.serve(), but it only works behind the unstable flag. Instead just throws, "Deno.serve is not a function".

@stale
Copy link

stale bot commented Oct 28, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 28, 2022
@lucacasonato lucacasonato added the bug Something isn't working correctly label Oct 29, 2022
@stale stale bot removed the stale label Oct 29, 2022
@bartlomieju
Copy link
Member

Since rewrite in 2846bbe I updated the original description, removing no longer relevant issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly
Projects
None yet
Development

No branches or pull requests

7 participants