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

Storage wrapper for locking #1

Closed
rullzer opened this issue Nov 7, 2019 · 5 comments
Closed

Storage wrapper for locking #1

rullzer opened this issue Nov 7, 2019 · 5 comments

Comments

@rullzer
Copy link
Member

rullzer commented Nov 7, 2019

This is needed to ensure the basic locking of the filesystem level

@icewind1991

@ArtificialOwl
Copy link
Member

Working fine, and implemented: locked file are not editable throw the Files API.
However, let's take Text App for example, the app will try to save every seconds after it fails (because file is locked). Meaning, the nextcloud.log is flooded (every seconds) of

@juliushaertl

{
  "reqId": "gV39X2IgPCu00yxdedxY",
  "level": 3,
  "time": "2019-11-08T16:15:35+00:00",
  "remoteAddr": "127.0.0.1",
  "user": "cult",
  "app": "text",
  "method": "POST",
  "url": "/index.php/apps/text/session/sync",
  "message": {
    "Exception": "OCP\\Files\\GenericFileException",
    "Message": "file_put_contents failed",
    "Code": 0,
    "Trace": [
      {
        "file": "/home/maxence/sites/nextcloud/server/apps/text/lib/Service/DocumentService.php",
        "line": 272,
        "function": "putContent",
        "class": "OC\\Files\\Node\\File",
        "type": "->",
        "args": [
          " ddasdaddd\n\nddddddssdddddddsdsadadsadsas"
        ]
      },
      {
        "file": "/home/maxence/sites/nextcloud/server/apps/text/lib/Service/ApiService.php",
        "line": 166,
        "function": "autosave",
        "class": "OCA\\Text\\Service\\DocumentService",
        "type": "->",
        "args": [
          68,
          29,
          " ddasdaddd\n\nddddddssdddddddsdsadadsadsas",
          false,
          false,
          null,
          "//New text document.md"
        ]
      },
      {
        "file": "/home/maxence/sites/nextcloud/server/apps/text/lib/Controller/SessionController.php",
        "line": 77,
        "function": "sync",
        "class": "OCA\\Text\\Service\\ApiService",
        "type": "->",
        "args": [
          68,
          50,
          "MJv0v9pwgiWHlTiBSTSMoAn957rGwhJe139IiH7E+gFdNPWBTPf7W4CUNL8hG6gQ",
          29,
          " ddasdaddd\n\nddddddssdddddddsdsadadsadsas",
          false,
          false
        ]
      },
      {
        "file": "/home/maxence/sites/nextcloud/server/lib/private/AppFramework/Http/Dispatcher.php",
        "line": 170,
        "function": "sync",
        "class": "OCA\\Text\\Controller\\SessionController",
        "type": "->",
        "args": [
          68,
          50,
          "MJv0v9pwgiWHlTiBSTSMoAn957rGwhJe139IiH7E+gFdNPWBTPf7W4CUNL8hG6gQ",
          29,
          " ddasdaddd\n\nddddddssdddddddsdsadadsadsas",
          false,
          false
        ]
      },
      {
        "file": "/home/maxence/sites/nextcloud/server/lib/private/AppFramework/Http/Dispatcher.php",
        "line": 99,
        "function": "executeController",
        "class": "OC\\AppFramework\\Http\\Dispatcher",
        "type": "->",
        "args": [
          {
            "__class__": "OCA\\Text\\Controller\\SessionController"
          },
          "sync"
        ]
      },
      {
        "file": "/home/maxence/sites/nextcloud/server/lib/private/AppFramework/App.php",
        "line": 124,
        "function": "dispatch",
        "class": "OC\\AppFramework\\Http\\Dispatcher",
        "type": "->",
        "args": [
          {
            "__class__": "OCA\\Text\\Controller\\SessionController"
          },
          "sync"
        ]
      },
      {
        "file": "/home/maxence/sites/nextcloud/server/lib/private/AppFramework/Routing/RouteActionHandler.php",
        "line": 47,
        "function": "main",
        "class": "OC\\AppFramework\\App",
        "type": "::",
        "args": [
          "OCA\\Text\\Controller\\SessionController",
          "sync",
          {
            "__class__": "OC\\AppFramework\\DependencyInjection\\DIContainer"
          },
          {
            "_route": "text.Session.sync"
          }
        ]
      },
      {
        "function": "__invoke",
        "class": "OC\\AppFramework\\Routing\\RouteActionHandler",
        "type": "->",
        "args": [
          {
            "_route": "text.Session.sync"
          }
        ]
      },
      {
        "file": "/home/maxence/sites/nextcloud/server/lib/private/Route/Router.php",
        "line": 298,
        "function": "call_user_func",
        "args": [
          {
            "__class__": "OC\\AppFramework\\Routing\\RouteActionHandler"
          },
          {
            "_route": "text.Session.sync"
          }
        ]
      },
      {
        "file": "/home/maxence/sites/nextcloud/server/lib/base.php",
        "line": 1006,
        "function": "match",
        "class": "OC\\Route\\Router",
        "type": "->",
        "args": [
          "/apps/text/session/sync"
        ]
      },
      {
        "file": "/home/maxence/sites/nextcloud/server/index.php",
        "line": 42,
        "function": "handleRequest",
        "class": "OC",
        "type": "::",
        "args": []
      }
    ],
    "File": "/home/maxence/sites/nextcloud/server/lib/private/Files/Node/File.php",
    "Line": 67,
    "CustomMessage": "--"
  },

@ArtificialOwl
Copy link
Member

the wrapper now returns a LockedException on UPDATE and DELETE permissions.
Meaning that while the Text App still tries to /sync every 5 seconds on a locked file, there is no flood of exception

@juliushaertl
Copy link
Member

@daita I think as a possible enhancement it would make sense to switch the text app to read only mode if the file is locked by a user that is not in the current editing session. Is the LockedException containing details about the lock?

@ArtificialOwl
Copy link
Member

we can improve https://github.com/nextcloud/server/blob/25565dd7d808dd33cc523525209ff5cfd5b9b6cd/lib/public/Lock/LockedException.php

Like, it seems I can already add a string $existingLock, however,its value is not stored.

  • we could store the value and add a getter, so you can get the token of the lock. My locks starts by files_lock/ so you can parse and check that the lock was generated by the App.

  • Now, If we have to add some code to the core, I would add a 4th parameter to the LockedException: $source with a getter so you can check the source of the lock. I would just set it to 'applock' or 'templock' so you know that the lock have been done on purpose. Even better, it could be an int set to -1 by default which represent the estimated time until automatic unlock.

@rullzer your thought ?

@ArtificialOwl ArtificialOwl reopened this Nov 22, 2019
@ArtificialOwl
Copy link
Member

ArtificialOwl commented Nov 23, 2019

I kept thinking that this is the best approach

nextcloud/server#18080

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants