-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Make sure all middlewares are only registered once #15385
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
Conversation
Signed-off-by: Joas Schilling <coding@schilljs.com>
| * @return boolean|null | ||
| */ | ||
| public function registerMiddleWare($middleWare) { | ||
| if (in_array($middleWare, $this->middleWares, true) !== false) { |
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.
So we registered the same middleware twice?
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.
yeah, all sharing middlewares were there twice
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 there a performance impact by this change? Could we use a key to reference the middleware and just replace it?
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.
In a default installation there are 14 (or with this bug 17) middlewares. Not really a huge impact.
blizzz
left a comment
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.
Fixes #15211
|
/backport to stable16 |
|
backport to stable16 in #15399 |
|
@hcderaad Did you run another command after applying the fix to get the broken shares back into working state? |
|
I am still facing the issue after Applying the changes on lib/private/AppFramework/DependencyInjection/DIContainer.php. |
|
The problem is that the patch needs to be applied on the server which sent you the federated share. |
|
I can confirm @nickvergessen comment. Once the NC instance owning the federated share was patched, my NC instance returned into a working state. However, I am concerned that another instance is able to bring down my instance. For example, I had to stop NC desktop and mobile client (desktop at least allowed me to uncheck the shared folder so I could continue using that client) or clients would keep crashing/stop working due to that error. I am lucky that the owner of the other instance was able to patch his instance. Now think about about an instance where the user has no control... Also, things like |
See #15765 |
Not sure where this origins from, but it broke federated shares for me.
With patch:
{ "data": { "id": 165, "parentId": 2, "mtime": 1557132779, "name": "wat", "permissions": 31, "mimetype": "httpd\/unix-directory", "size": 0, "type": "dir", "etag": "5ccff5eb2c45d", "children": [ ] }, "status": "success" }Without:
{ "data": { "data": { "id": 165, "parentId": 2, "mtime": 1557132779, "name": "wat", "permissions": 31, "mimetype": "httpd\/unix-directory", "size": 0, "type": "dir", "etag": "5ccff5eb2c45d", "children": [ ] }, "status": "success" }, "status": "success" }The double nesting causes broken SQL queries when tryiung to store the cache information for
statusanddata....