Add possibility to upgrade the current session to a new TCP handler #75

mfelsche · 2024-02-15T23:23:52Z

and add a Websocket example demonstrating this new feature.

SeanTAllen · 2024-02-20T19:00:06Z

examples/websocket_echo_server/lock.json

@@ -0,0 +1,32 @@
+{


can we not commit the lock.json? that tends to lead to wtf with tests etc.

is this required? i assume it isn't. (if we do remove, lets add it to the .gitignore)

This is in the example. We can totally keep it out. My thought was that it is usually good practice to include it to improve reproducability of the build of this corral project, as the lock is where the version/git-tag pinning happens.

Ive had to do chasing down lock files to find broken builds related to examples that otherwise would have worked. I see your point, its a tough one. They can both lead to pain, the "having lock file" is fresher for pain for me.

http_server/request.pony

http_server/session.pony

SeanTAllen · 2024-02-20T19:03:15Z

@mfelsche seems like we should add github.com/mfelsche/pony-websocket as a ponylang project. Are you up for doing that?

SeanTAllen · 2024-02-20T19:04:17Z

Or perhaps @mfelsche, would it make sense to incorporate github.com/mfelsche/pony-websocket into this library?

SeanTAllen · 2024-02-20T19:05:36Z

This needs release notes.

SeanTAllen · 2024-02-20T19:06:36Z

examples/websocket_echo_server/lock.json

+    {
+      "locator": ".",
+      "revision": "main"
+    },


I'd rather we use "../" in the use in examples.

Makefile

jemc · 2024-02-20T19:26:07Z

examples/websocket_echo_server/main.pony

+        _session.send_raw(
+          _response_builder.set_status(StatusSwitchingProtocols)
+            .add_header("Upgrade", "websocket")
+            .add_header("Connection", "Upgrade")
+            .add_header("Sec-WebSocket-Accept", sec_ws_accept)
+            .finish_headers()
+            .build(),
+          request_id
+        )


Should this be integrated be integrated with the _session.upgrade method so that this "Switching Protocols" response will definitely happen (without the user needing to remember to implement this)?

I think it would be better to make this all in one call rather than asking the protocol switching handler to construct a raw response.

Are websockets the only reason one would upgrade the session, or might there be other reasons and the header would look different?

Upgrade is a general part of the HTTP protocol, so it isn't websocket specific.

I thought about integrating sending a 101 Response with in the Session.upgrade_protocol function. I initially voted against it, because i wanted to keep the library as raw and low-level as possible. While nearly all protocol upgrades will be used for Websockets, I'd rather not block the path for upgrading to other protocols. And people might want to add special headers to their response and we have multiple API styles used in this library for sending responses, so a generic upgrade_protocol method that does all the upgrading boilerplate while still being generic enough so people can change the response as they see fit, didn't seem worthwile to me.

I'd suggest to move the discussion about a more higher-level API for this to new Issue if need be.

SeanTAllen · 2024-02-20T19:28:23Z

If the websocket library isn't a ponylang project then the example shouldnt be part of this repo. The build could be broken by the websocket library falling out of date and maintainers for this package who aren't Matthias wouldn't be able to fix without removing the example.

I think either example goes or we commit to making the websocket library a ponylang project.

IF we go for "example goes", then I think we should take the example and it can be an example repo unto itself OR we can do a blog post on the website after this is released with an example (and perhaps some FAQ fun on the website).

http_server/session.pony

Co-authored-by: Joe Eli McIlvain <[email protected]>

SeanTAllen · 2024-02-27T18:36:53Z

@mfelsche for the largest question first so we can move this along, do you want to pull the example out that relies on an external 3rd party library out and use some other location to detail the example or do you want to have the websockets library become a ponylang project that we make sure works with pony language changes going forward?

sacovo · 2024-03-01T09:27:02Z

Maybe remove the example for now, and if/when the websocket library is a ponylang project it can be added back in?

mfelsche · 2024-03-01T19:17:39Z

Sorry for the late reply. In order to move this forward, lets for now move this example out. I will improve the documentation of the upgrade_protocol behaviour. @sacovo Does this as is solve your use case for websocket support?

sacovo · 2024-03-01T19:24:51Z

Yes, it does. Thank you for your work!

for representing the HTTP version as 8 byte number for faster comparison during parsing.

stdlib first, external packages second. Alphabetical order in each section.

SeanTAllen · 2024-03-05T19:05:19Z

Makefile

+EXAMPLES := $(notdir $(shell find $(EXAMPLES_DIR)/* -maxdepth 0 -type d -not -name websocket_echo_server))
+EXAMPLES_SOURCE_FILES := $(shell find $(EXAMPLES_DIR) -name *.pony -not -path '**/websocket_echo_server/*')


This should be rolled back to how it was previously.

SeanTAllen · 2024-03-05T19:06:05Z

http_server/_server_connection.pony

-use "net"
 use "collections"
-use "valbytes"
 use "debug"
+use "net"
 use "time"

+use "valbytes"


This is unrelated at this point, no new uses were added so I think this should be rolled bacl

SeanTAllen · 2024-03-05T19:06:15Z

http_server/_server_conn_handler.pony

-use "net"
 use "debug"
+use "net"


This is unrelated at this point, no new uses were added so I think this should be rolled back

SeanTAllen · 2024-03-05T19:06:38Z

http_server/_test.pony

-use "pony_check"
-use "pony_test"
-use "net"
+use "buffered"
 use "collections"


This is unrelated at this point, no new uses were added so I think this should be rolled back

SeanTAllen · 2024-03-05T19:06:52Z

http_server/_test_connection_handling.pony

@@ -1,5 +1,5 @@
-use "pony_test"


This is unrelated at this point, no new uses were added so I think this should be rolled back

SeanTAllen · 2024-03-05T19:07:06Z

http_server/_test_headers.pony

@@ -1,6 +1,7 @@
 use "debug"
 use "pony_check"


This is unrelated at this point, no new uses were added so I think this should be rolled back

SeanTAllen · 2024-03-05T19:07:19Z

http_server/_test_pipelining.pony

@@ -1,7 +1,8 @@
 use "net"
-use "time"
 use "pony_test"
 use "random"


This is unrelated at this point, no new uses were added so I think this should be rolled back

SeanTAllen · 2024-03-05T19:07:31Z

http_server/_test_request_parser.pony

@@ -1,6 +1,7 @@
 use "debug"
 use "itertools"
 use "pony_test"
+


this should be rolled back

SeanTAllen · 2024-03-05T19:08:34Z

http_server/request_parser.pony

-use "valbytes"
 use "debug"

+use "valbytes"
+


This is unrelated at this point, no new uses were added so I think this should be rolled back

SeanTAllen · 2024-03-05T19:08:42Z

http_server/response.pony

-use "valbytes"
 use "format"

+use "valbytes"
+
 interface val Response is ByteSeqIter


This is unrelated at this point, no new uses were added so I think this should be rolled back

SeanTAllen · 2024-03-05T19:08:53Z

http_server/server.pony

 use "collections"
+use "debug"
 use "net"
 use "net_ssl"
 use "time"
-use "debug"


This is unrelated at this point, no new uses were added so I think this should be rolled back

SeanTAllen · 2024-03-05T19:09:25Z

http_server/sync_handler.pony

-use "valbytes"
 use "debug"

+use "valbytes"
+


This is unrelated at this point, no new uses were added so I think this should be rolled back

.release-notes/next-release.md

Co-authored-by: Joe Eli McIlvain <[email protected]>

This reverts commit 8e8d9f7.

Add possibility to upgrade the current session to a new TCP handler

67094fe

and add a Websocket example demonstrating this new feature.

ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Feb 15, 2024

This was referenced Feb 15, 2024

Add special handling for parsing websocket data #73

Closed

Expose tcp connection notify for usage with already open TCP connections oraoto/pony-websocket#12

Open

mfelsche added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Feb 15, 2024

SeanTAllen reviewed Feb 20, 2024

View reviewed changes

http_server/request.pony Outdated Show resolved Hide resolved

SeanTAllen reviewed Feb 20, 2024

View reviewed changes

http_server/session.pony Show resolved Hide resolved

SeanTAllen added the do not merge This PR should not be merged at this time label Feb 20, 2024

SeanTAllen reviewed Feb 20, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

SeanTAllen reviewed Feb 20, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

jemc reviewed Feb 20, 2024

View reviewed changes

http_server/session.pony Outdated Show resolved Hide resolved

rename Session.upgrade to Session.upgrade_protocol

afd3af0

Co-authored-by: Joe Eli McIlvain <[email protected]>

mfelsche added 7 commits March 2, 2024 21:36

Improve documentation for Session.upgrade_protocol

1bf1354

Remove websocket example

78c7e02

Fix typoes in http_server documentation

e13a3c2

Document Version.u64() method

8bfeef9

for representing the HTTP version as 8 byte number for faster comparison during parsing.

Reorder imports

559d1e2

Reorder imports

8e8d9f7

stdlib first, external packages second. Alphabetical order in each section.

Add release notes

de404e1

SeanTAllen reviewed Mar 5, 2024

View reviewed changes

jemc reviewed Mar 5, 2024

View reviewed changes

.release-notes/next-release.md Show resolved Hide resolved

SeanTAllen removed the do not merge This PR should not be merged at this time label Mar 5, 2024

mfelsche and others added 4 commits March 7, 2024 20:30

Update .release-notes/next-release.md

3ed3ed8

Co-authored-by: Joe Eli McIlvain <[email protected]>

Revert "Reorder imports"

931c9b7

This reverts commit 8e8d9f7.

revert changes in Makefile

cbbd04e

add lock.json to .gitignore

b22ef3e

SeanTAllen approved these changes Mar 12, 2024

View reviewed changes

Merge branch 'main' into session_upgrade

d0d800e

jemc approved these changes Mar 12, 2024

View reviewed changes

SeanTAllen merged commit 0983542 into main Mar 12, 2024
7 checks passed

SeanTAllen deleted the session_upgrade branch March 12, 2024 18:06

ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Mar 12, 2024

github-actions bot pushed a commit that referenced this pull request Mar 12, 2024

Update CHANGELOG for PR #75

05df9e2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possibility to upgrade the current session to a new TCP handler #75

Add possibility to upgrade the current session to a new TCP handler #75

mfelsche commented Feb 15, 2024

SeanTAllen Feb 20, 2024

mfelsche Feb 20, 2024

SeanTAllen Feb 20, 2024

SeanTAllen commented Feb 20, 2024

SeanTAllen commented Feb 20, 2024

SeanTAllen commented Feb 20, 2024

SeanTAllen Feb 20, 2024

jemc Feb 20, 2024

sacovo Feb 22, 2024

SeanTAllen Feb 27, 2024

mfelsche Mar 2, 2024

SeanTAllen commented Feb 20, 2024

SeanTAllen commented Feb 27, 2024

sacovo commented Mar 1, 2024

mfelsche commented Mar 1, 2024

sacovo commented Mar 1, 2024

SeanTAllen Mar 5, 2024

SeanTAllen Mar 5, 2024

SeanTAllen Mar 5, 2024

SeanTAllen Mar 5, 2024

SeanTAllen Mar 5, 2024

SeanTAllen Mar 5, 2024

SeanTAllen Mar 5, 2024

SeanTAllen Mar 5, 2024

SeanTAllen Mar 5, 2024

SeanTAllen Mar 5, 2024

SeanTAllen Mar 5, 2024

SeanTAllen Mar 5, 2024

		EXAMPLES := $(notdir $(shell find $(EXAMPLES_DIR)/* -maxdepth 0 -type d -not -name websocket_echo_server))
		EXAMPLES_SOURCE_FILES := $(shell find $(EXAMPLES_DIR) -name .pony -not -path '/websocket_echo_server/')

Add possibility to upgrade the current session to a new TCP handler #75

Add possibility to upgrade the current session to a new TCP handler #75

Conversation

mfelsche commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Feb 20, 2024

SeanTAllen commented Feb 20, 2024

SeanTAllen commented Feb 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Feb 20, 2024

SeanTAllen commented Feb 27, 2024

sacovo commented Mar 1, 2024

mfelsche commented Mar 1, 2024

sacovo commented Mar 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment