[Try] HTTP based PHP signaling server for colaborative editing #53922
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jorgefilipecosta
added
the
[Feature] Real-time Collaboration
|
Warning: Type of PR label error To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. |
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/experimental/sync/README.md ❔ lib/experimental/sync/class-gutenberg-http-signaling-server.php ❔ lib/load.php |
|
Size Change: +814 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
jorgefilipecosta
changed the title
jorgefilipecosta
changed the title
youknowriad
reviewed
Aug 25, 2023
youknowriad
reviewed
Aug 25, 2023
youknowriad
reviewed
Aug 25, 2023
19 tasks
youknowriad
requested review from
a team and
scruffian
and removed request for
a team
tyxla
reviewed
Sep 8, 2023
There was a problem hiding this comment.
This appears like a serious, ambitious project! Nice work!
I've left a drive-by review, but it feels like much of it could be rearchitected, split into smaller, more digestible pieces, so it could be better reviewed and maintained in the future. It could also benefit from some unit tests, which would definitely use that prior rearchitecting work.
What I've found to work well with large PRs like this is is using such a PR as a PoC and then landing various pieces of it, as they're being rewritten to be more modular and maintainable, and with tests and documentation added in the meantime.
lib/sync/endpoint.php
Outdated
| function _gutenberg_save_contents_to_file_descriptor( $fd, $content ) { | ||
| rewind( $fd ); | ||
| $data = serialize( $content ); | ||
| fwrite( $fd, $data ); |
There was a problem hiding this comment.
We might have to do some additional checks before fwrite to ensure that we can actually write. Otherwise, fwrite will raise a warning on failure. This applies to any other file operation that is potentially risky (because of lack of permissions or any other reason).
lib/sync/endpoint.php
Outdated
| flock( $fd_topics_subscriber, LOCK_EX ); | ||
| $topics_to_subscribers = _gutenberg_get_contents_from_file_descriptor( $fd_topics_subscriber ); | ||
|
|
||
| switch ( $message['type'] ) { |
There was a problem hiding this comment.
The message handling is a good example of where OOP structure could help - each message type could be handled by its own class.
jorgefilipecosta
force-pushed
the
add/sync-php-signaling-2
branch
4 times, most recently
from
271fe53 to
9e49f2d
Compare
|
Flaky tests detected in 7474a70. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6481871607
|
youknowriad
reviewed
Sep 18, 2023
There was a problem hiding this comment.
I really like the documentation about the signaling server. So it's more a generic way to exchange data between different clients connected to WordPress.
I might be asking too much but I wonder if there's specifics about the usage we make of this server to connect peers and get WebRTC "addresses" or something. I wonder if that part is worth documenting too (maybe elsewhere)
tyxla
reviewed
Sep 18, 2023
There was a problem hiding this comment.
I know this is already a big project, but one aspect I think would be good to consider eventually is extensibility. The way I see it is having a common interface for communication, and then every transport method would implement that interface. That way core, plugins, and 3rd parties would all have the opportunity to build custom transport mechanisms that are compatible and ultimately interchangeable with the default core one. @mtias has also been talking about that in the collaborative editing planning documents:
The architecture should be pluggable so extenders can provide other solutions.
Source: https://make.wordpress.org/core/2023/07/03/real-time-collaboration/
|
@tyxla Having the ability to replace the transport layer of the P2P collaboration is definitely something we should consider. It's already tracked in the overview issue. #52593 That said, I agree that it seems to be a separate concern that can be addressed in a separate PR. There's so many things that are still left for the sync engine. |
jorgefilipecosta
force-pushed
the
add/sync-php-signaling-2
branch
3 times, most recently
from
9fbdac5 to
956513d
Compare
jorgefilipecosta
force-pushed
the
add/sync-php-signaling-2
branch
from
829cb36 to
8fa9d7e
Compare
Co-authored-by: André <583546+oandregal@users.noreply.github.com>
oandregal
reviewed
Oct 19, 2023
oandregal
reviewed
Oct 19, 2023
| */ | ||
| public static function do_wp_ajax_action() { | ||
| if ( empty( $_REQUEST ) || empty( $_REQUEST['subscriber_id'] ) ) { | ||
| die( 'no identifier' ); |
There was a problem hiding this comment.
Now that we have errors, what do you think of sending this as messages back to the client instead of silently dying? There's a few in this method. It can be a follow-up.
oandregal
force-pushed
the
add/sync-php-signaling-2
branch
from
682032e to
b42e5f1
Compare
|
Rebased because there was a conflict with #55260 and I'm also updating the docs. |
oandregal
approved these changes
Oct 19, 2023
There was a problem hiding this comment.
I've done some testing and this is working as expected:
Gravacao.do.ecra.2023-10-19.as.13.55.59.mov
The flow at the server and the different supported operations are clear. It's also documented. I find this to be the key part of this PR.
I'd have some suggestions for client-side code, though I see it's organized in a way that enables us to compare the changes with the actual y-webrtc codebase. I guess the goal is to propose a pull request for that project to support a HTTP-based signaling server.
|
Hey folks, I believe this introduced a permafailing PHPUnit test. (See https://github.com/WordPress/gutenberg/actions/runs/6573611811/job/17859027200). Or at least, the trunk commits before this one were passing :) Edit: Looks like there was a small dangling comma in this PR causing a syntax error! Fix here: #55494 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR proposes a signaling server for WebRTC connections using PHP and HTTP. The work relies on the y-webrtc repository. Since certain required methods were not exported, we copied code from the y-webrtc package, adding a few exports within the y-webrtc folder.
This work employs WordPress Ajax requests to send messages to the server, and an EventSource to periodically retrieve messages. We have created a new class, HttpSignalingConn, to handle this type of signaling, along with a straightforward PHP signaling server. This server attempts to manage race conditions, ensuring message integrity.
Currently, we use temporary files to store messages on the server across distinct communication channels/clients. In the future, switching to a database might be logical, considering that in some WordPress setups, writing files might be unfeasible. However, for now, files offer greater flexibility and faster iteration.
cc: @youknowriad
Testing