-
-
Notifications
You must be signed in to change notification settings - Fork 487
[4.x] BroadcastingConfigBootstrapper: fix central instances persisting in tenant context #1448
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
Open
lukinovec
wants to merge
64
commits into
master
Choose a base branch
from
broadcasting-fixes
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
64 commits
Select commit
Hold shift + click to select a range
7d749eb
BroadcastingConfigBootstrapper: test mapping credentials
lukinovec 2ecc94d
BroadcastingConfigBootstrapper: test persistence of custom driver cre…
lukinovec fafd082
Fix typo in test
lukinovec c653c51
BroadcastingConfigBootstrapper: make tenant manager inherit central m…
lukinovec b1e91f1
BroadcastingConfigBootstrapper: make `Broadcaster::class` resolve to …
lukinovec 65beecf
BroadcastingConfigBootstrapper: clear the `Broadcast` facade's resolv…
lukinovec 0b860ea
Fix code style (php-cs-fixer)
github-actions[bot] c4f4451
Add assertions for the config of the bound manager's driver
lukinovec d939866
Fix custom creator assertions
lukinovec 28b6119
TenancyBroadcastManager: update docblocks
lukinovec 0fbe1bc
TenancyBroadcastManager: delete `Broadcaster` singleton binding
lukinovec b6c035c
Improve comments
lukinovec bbe2ff0
BroadcastingTest: update channel inheritance test
lukinovec b2add06
Delete BroadcastingTest
lukinovec 9e9bedc
Fix code style (php-cs-fixer)
github-actions[bot] 4b1cc9c
Improve comments
lukinovec fc45e09
Update BroadcastingConfigBootstrapperTest
lukinovec 6b99921
BroadcastingConfigBootstrapper: correct `$credentialsMap` array_merge…
lukinovec 29dd23d
BroadcastingConfigBootstrapperTest: add 'reverb' driver to datasets
lukinovec 4937a74
BroadcastingConfigBootstrapper and TenancyBroadcastManager: comments
lukinovec c831393
Update comment
lukinovec ef476c5
Polish comments
lukinovec f8528fc
Add 'reverb' to `TenancyBroadcastManager::$tenantBroadcasters`
lukinovec dc344b7
Merge branch 'master' into broadcasting-fixes
stancl 9ab0a72
Fix typo
lukinovec 4aeaa66
Remove unused `$broadcaster` parameter
lukinovec b3d5197
Correct channel prefix test comments
lukinovec a247bb0
Correct config bootstrapper test comment
lukinovec 20d494f
Reset `BroadcastingConfigBootstrapper::$broadcaster` in tests
lukinovec 7af2b8a
Improve BroadcastManager instance checks
lukinovec a3c5568
Add regression test for credentialsMap merge order
lukinovec f3652a8
Make `with()` formatting consistent
lukinovec a7acd07
Reset `BroadcastingConfigBootstrapper::$mapPresets` in tests
lukinovec 7db486d
Extract cleanup in broadcasting config test file
lukinovec d885659
Use toEqualCanonicalizing instead of toBe for custom creators
lukinovec e592b37
Merge branch 'master' into broadcasting-fixes
lukinovec ddd8c68
Improve comments
lukinovec 614344a
Add test for reverting the bound broadcaster when tenancy ends
lukinovec f20f801
Merge branch 'master' into broadcasting-fixes
lukinovec 9af1735
Explain why we swap the bound Broadcaster singleton better, remove th…
lukinovec 937c8c8
Delete the redundant BroadcastingFactory::class arg from the Broadcas…
lukinovec a19d3b5
Merge branch 'master' into broadcasting-fixes
lukinovec 24aeb23
Remove unused `$app` param and `?BroadcastManager` typehint (`singlet…
lukinovec 2a0464d
Resolve (and cache) tenant broadcasters once per tenancy initializati…
lukinovec d80fed1
Remove stale $tenantBroadcasters mention
lukinovec af14d40
Delete extra newline, fix comment inconsistency
lukinovec b8fddf8
Retrieve the re-registered channel closure to assert what tenant_chan…
lukinovec 128a1ad
Improve BroadcastingConfigBootstrapper docblock
lukinovec 9e6e0a9
Remove TenancyBroadcastManager, pass the channel auth closures direct…
lukinovec 5911619
Simplify channel-passing test
lukinovec 120cec4
Add spaces after `fn`/`function` for consistency
lukinovec 12e51d0
Test that when a tenant doesn't have the property that should be mapp…
lukinovec 0ba588d
Improve channel closure inheritance test
lukinovec e6c4c31
Test that using broadcasters that only implement the Broadcaster cont…
lukinovec 4422df2
Test configuring BroadcastingConfigBootstrapper::$broadcaster
lukinovec 1409d72
Make test name more specific
lukinovec 31d1663
Fix typo in comment
lukinovec d535b28
Update src/Bootstrappers/BroadcastingConfigBootstrapper.php
lukinovec 81d41b4
Use `$centralBroadcaster->getChannels()` to get the channels instead …
lukinovec c07996e
Explain extending BroadcasterContract better
lukinovec 44b97ae
phpstan fix: comment ignore
stancl 9efebf9
Test reverting to central credentials after a tenant with overrides
lukinovec 50afc49
Test that the authenticated user resolvers get copied to tenant broad…
lukinovec fe7468a
Copy the broadcaster auth state directly
lukinovec File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file was deleted.
Oops, something went wrong.
This file contains hidden or 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the reason for creating the instance manually like this that if we simply forgot the bound instance from the container, the custom creators that were already registered would be lost?
To confirm - when would
BroadcastManager::extend()be used? Is this change simply because of how we do things in our tests or would real applications also extend the manager before tenancy bootstrap?And what are some common use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, yeah
I made the change because when I reviewed the tests, I realized that this could be a real problem with any custom driver.
Custom drivers are registered via
Broadcast::extend('foobar', ...)in a service provider'sboot(), which runs before tenancy is initialized, so in a real app the creators are always on the central manager before bootstrap. Without copying them, a tenant using a custom driver as the default broadcaster would get "Driver [foobar] is not supported" in the tenant context.Writing your own driver, or installing a package that ships one. I see that for example, the Centrifugo package registers its driver exactly like this, in its provider's
boot()(it callsextend()on an injectedBroadcastManagerinstead of the facade, but that's the same thing sinceBroadcast::extend()proxies to the manager): https://github.com/denis660/laravel-centrifugo/blob/bec36ea8323fb56dff42bfc3999600abc567afaf/src/CentrifugoServiceProvider.php#L27-L29There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we are recreating the manager from scratch, and then copying existing stuff into the new manager, instead of trying to reset parts of the manager - if possible. The ones that contain state that becomes utdated in the tenant context.
As a brief test, I looked at what
forget*methods there are in the manager and what properties exist there. I sawforgetDrivers()and that there aren't really many other properties that could hold outdated state. If I replace this whole thing with justforgetDrivers()on the boundBroadcastManager, all tests except two assertions that are coupled to this specific implementation seem to pass.Have we tried that before and was it insufficient to make everything work? Wondering why we'd be re-instantiating managers and copying private properties to new instances and things like that in this bootstrapper when we normally just try to reset state.
Thought of this when I looked at the
invade()-related review since I checked where we use invade and it's only this bootstrapper and some tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this too -- these tests are coupled to the specific implementation, so yeah, these fail. Other tests pass because we don't deal with the custom creator inheritance, the
BroadcastManagerextend andforgetDrivershave similar primary purpose (= make the manager resolve fresh broadcasters).(actually, I think there's also a gap in the tests -- we don't have a test for whether revert restores the central credentials after a tenant with overrides, and that one would fail with
forgetDrivers()-- more on that below)But
forgetDriversis problematic.I don't think we have tried that at all because before, we were swapping the bound broadcast manager for the custom TenancyBroadcastManager (so there was no reason to try that). And after we removed that class, it didn't occur to me to test using just
forgetDrivers()instead of theextend().forgetDrivers()keeps the custom creators (since it only clears$drivers), so custom drivers would still work without the need to copy anything. The only custom creator difference is that a creator registered inside a tenant context would leak into later contexts instead of being isolated per tenant (that's why one of the tests fails), but that's an edge case.But the worse thing is that we have to properly
revert()whatbootstrap()does. If we just didforgetDrivers()on the bound manager inbootstrap(), things will work just fine in tenant context (if we ignore custom creators). Then onrevert(), if we didforgetDrivers(), we'd discard the tenant broadcaster, but nothing would copy the channel closures back to the central broadcaster. To make the fullforgetDrivers()implementation actually correct, we'd have to copy the channel closures onto the rebuilt central broadcaster on revert too.And if instead we kept
instance()on revert (notforgetDrivers()), the manager's cached default driver would still hold the previous (tenant) broadcaster, so /broadcasting/auth in central context would use the old tenant's key (the channel closures were copied over during bootstrap, so /broadcasting/auth wouldn't 403, but the response would be signed using the tenant's key, and the websocket server would just reject it).So both revert solutions need copying extra state to be correct, which is why extend() (restoring the untouched central instances via instance()) probably ends up cleaner. I'll try how that implementation could look, but like I wrote, I think the
extend()may be cleaner in the end -- I'll let you know.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Keeping all the changes local for now since this review is tied to #1448 (comment), and we'll be doing more testing/code changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added assertions for this to the credential mapping test. They fail with the forgetDrivers() thing we tested in bootstrap(), pass with extend().
9efebf9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried it, and it's like I wrote --
bootstrap()gets simpler (forgetDrivers()+ the same auth state copying), butrevert()has to re-resolve the central broadcaster and copy the auth state back onto it:So the copying happens in both directions instead of once (meaning
invade()stays too), the original broadcaster isn't restored on revert -- a fresh instance replaces it, so instances resolved before initializing tenancy end up stale. This all would be a bit more messy than the current implementation, so imo, switching to this won't be worth it.(About the new assertions I mentioned in the previous comment -- with the full implementation, these assertions pass too, since
revert()re-resolves the broadcaster with the central config -- only the instance check assertions fail)Also tested the "resetting the existing object" idea from the other review, that's more interesting. I'll comment on that in a minute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #1448 (comment)