Skip to content
Snippets Groups Projects
  1. Oct 20, 2022
    • Matt Harbison's avatar
      lfs: improve an exception message for blob corruption detected on transfer · 250d9c8aaf10
      Matt Harbison authored
      The message about the server crash originated in 0ee0a3f6a990 (after support for
      serving blobs was added), but was copied from the Facebook repo that forked
      prior to server side support.  Therefore, this message only displayed in their
      client, so it was safe to assume the server crashed.  But that was never the
      case for vanilla Mercurial, as I saw this in a server log.
      
      Also, display the blob reference so that it's easier to figure out where the
      problem was when a bunch of blobs are transferred at once.
      250d9c8aaf10
    • Matt Harbison's avatar
      lfs: fix interpolation of int and %s in an exception case · 192949b68159
      Matt Harbison authored
      Seen in the wild in a server log when MS antivirus was quarantining a file on
      the client side.
      192949b68159
  2. Feb 21, 2022
  3. Mar 03, 2022
  4. Apr 12, 2021
  5. Dec 01, 2020
  6. Nov 27, 2020
    • durin42's avatar
      formating: upgrade to black 20.8b1 · 89a2afe31e82
      durin42 authored
      This required a couple of small tweaks to un-confuse black, but now it
      works. Big formatting changes come from:
      
       * Dramatically improved collection-splitting logic upstream
       * Black having a strong (correct IMO) opinion that """ is better than '''
      
      Differential Revision: https://phab.mercurial-scm.org/D9430
      89a2afe31e82
  7. Feb 09, 2020
    • Matt Harbison's avatar
      lfs: use str for the open() mode when opening a blob for py3 · 234001d22ba6
      Matt Harbison authored
      The other fix for this was to leave the mode as bytes, and import
      `pycompat.open()` like a bunch of other modules do.  But I think it's confusing
      to still use bytes at the python boundary, and obviously error prone.  Grepping
      for ` open\(.+, ['"][a-z]+['"]\)` and ` open\(.+, b['"][a-z]+['"]\)` outside of
      `tests`, there are 51 and 87 uses respectively, so it's not like this is a rare
      direct usage.
      
      Differential Revision: https://phab.mercurial-scm.org/D8099
      234001d22ba6
  8. Jan 21, 2020
    • Matt Harbison's avatar
      lfs: fix the stall and corruption issue when concurrently uploading blobs · 43eea17ae7b3
      Matt Harbison authored
      We've avoided the issue up to this point by gating worker usage with an
      experimental config.  See 10e62d5efa73, and the thread linked there for some of
      the initial diagnosis, but essentially some data was being read from the blob
      before an error occurred and `keepalive` retried, but didn't rewind the file
      pointer.  So the leading data was lost from the blob on the server, and the
      connection stalled, trying to send more data than available.
      
      In trying to recreate this, I was unable to do so uploading from Windows to
      CentOS 7.  But it reproduced every time going from CentOS 7 to another CentOS 7
      over https.
      
      I found recent fixes in the FaceBook repo to address this[1][2].  The commit
      message for the first is:
      
          The KeepAlive HTTP implementation is bugged in it's retry logic, it supports
          reading from a file pointer, but doesn't support rewinding of the seek cursor
          when it performs a retry. So it can happen that an upload fails for whatever
          reason and will then 'hang' on the retry event.
      
          The sequence of events that get triggered are:
           - Upload file A, goes OK. Keep-Alive caches connection.
           - Upload file B, fails due to (for example) failing Keep-Alive, but LFS file
             pointer has been consumed for the upload and fd has been closed.
           - Retry for file B starts, sets the Content-Length properly to the expected
             file size, but since file pointer has been consumed no data will be uploaded,
             causing the server to wait for the uploaded data until either client or
             server reaches a timeout, making it seem as our mercurial process hangs.
      
          This is just a stop-gap measure to prevent this behavior from blocking Mercurial
          (LFS has retry logic). A proper solutions need to be build on top of this
          stop-gap measure: for upload from file pointers, we should support fseek() on
          the interface. Since we expect to consume the whole file always anyways, this
          should be safe. This way we can seek back to the beginning on a retry.
      
      I ported those two patches, and it works.  But I see that `url._sendfile()` does
      a rewind on `httpsendfile` objects[3], so maybe it's better to keep this all in
      one place and avoid a second seek.  We may still want the first FaceBook patch
      as extra protection for this problem in general.  The other two uses of
      `httpsendfile` are in the wire protocol to upload bundles, and to upload
      largefiles.  Neither of these appear to use a worker, and I'm not sure why
      workers seem to trigger this, or if this could have happened without a worker.
      
      Since `httpsendfile` already has a `close()` method, that is dropped.  That
      class also explicitly says there's no `__len__` attribute, so that is removed
      too.  The override for `read()` is necessary to avoid the progressbar usage per
      file.
      
      [1] https://github.com/facebookexperimental/eden/commit/c350d6536d90c044c837abdd3675185644481469
      [2] https://github.com/facebookexperimental/eden/commit/77f0d3fd0415e81b63e317e457af9c55c46103ee
      [3] https://www.mercurial-scm.org/repo/hg/file/5.2.2/mercurial/url.py#l176
      
      Differential Revision: https://phab.mercurial-scm.org/D7962
      43eea17ae7b3
    • Matt Harbison's avatar
      lfs: add a method to the local blobstore to convert OIDs to file paths · 06de4a673f48
      Matt Harbison authored
      This is less ugly than passing an open callback to the `httpsendfile`
      constuctor.
      
      Differential Revision: https://phab.mercurial-scm.org/D7961
      06de4a673f48
    • Matt Harbison's avatar
      lfs: move the initialization of the upload request into the try block · 46c8f15fb2b4
      Matt Harbison authored
      This (almost) guarantees that the file is closed in the case of an exception.
      The one hole is if the `seek(SEEK_END)`/`tell()`/`seek(0)` sequence fails.  But
      that's going to go away when subclassing `httpconnection.httpsendfile` to fix
      the worker problem, so I'm not going to worry too much.  (And that class appears
      to have the same problem.)
      
      Differential Revision: https://phab.mercurial-scm.org/D7959
      46c8f15fb2b4
    • Matt Harbison's avatar
      lfs: drop an unnecessary r'' prefix · b2408acaa4c9
      Matt Harbison authored
      No longer necessary since the source transformer was removed.
      
      # skip-blame for changing string prefixes
      
      Differential Revision: https://phab.mercurial-scm.org/D7958
      b2408acaa4c9
    • Matt Harbison's avatar
      lfs: explicitly close the file handle for the blob being uploaded · 2ad4e8aefcf4
      Matt Harbison authored
      The previous code relied on reading the blob fully to close it.  The obvious
      problem is if an error occurs before that point.  But there is also a problem
      when using workers where the data may need to be re-read, which can't happen
      once it is closed.  This eliminates the surprising behavior before attempting to
      fix the worker problem.
      
      Differential Revision: https://phab.mercurial-scm.org/D7957
      2ad4e8aefcf4
    • Matt Harbison's avatar
      lfs: drop the unused progressbar code in the `filewithprogress` class · 5f841daf3b41
      Matt Harbison authored
      This has been unused since f98fac24b757, which added worker based transfers for
      concurrency, shifting the progressbar maintenance to the single thread waiting
      on the worker to complete.  Since the name no longer fits, rename the class.
      
      Differential Revision: https://phab.mercurial-scm.org/D7956
      5f841daf3b41
  9. Jan 15, 2020
  10. Jan 14, 2020
  11. Nov 08, 2019
    • Augie Fackler's avatar
      cleanup: remove pointless r-prefixes on single-quoted strings · 9f70512ae2cf
      Augie Fackler authored
      This is the promised second step on single-quoted strings. These had
      existed because our source transformer didn't turn r'' into b'', so we
      had tagged some strings as r-strings to get "native" strings on both
      Pythons. Now that the transformer is gone, we can dispense with this
      nonsense.
      
      Methodology:
      
      I ran
      
          hg locate 'set:added() or modified() or clean()' | egrep '.*\.py$'  | xargs egrep --color=never -n  -- \[\^b\]\[\^a-z\]r\'\[\^\'\\\\\]\*\'\[\^\'\
      
      in an emacs grep-mode buffer, and then used a keyboard macro to
      iterate over the results and remove the r prefix as needed.
      
      # skip-blame removing unneeded r prefixes left over from Python 3 migration.
      
      Differential Revision: https://phab.mercurial-scm.org/D7306
      9f70512ae2cf
  12. Nov 02, 2019
    • Gregory Szorc's avatar
      py3: define and use json.loads polyfill · 579672b347d2
      Gregory Szorc authored
      Python 3.5's json.loads() requires a str. Only Python 3.6+
      supports passing a bytes or bytearray.
      
      This commit implements a json.loads() polyfill on Python 3.5
      so that we can use bytes. The added function to detect encodings
      comes verbatim from Python 3.7.
      579672b347d2
  13. Oct 06, 2019
  14. Feb 07, 2019
    • Matt Harbison's avatar
      lfs: disable all authentication except Basic for HTTP(S) connections · 698667eb7523
      Matt Harbison authored
      I ran into a problem pushing to an old Apache server- the normal outgoing
      traffic occurred, the Batch API request and response occurred, and then things
      suddenly halted.  5 minutes later, a 500 was returned, and the server log had a
      timeout reading 32K from `self._req.bodyfh` in hgweb.request.sendresponse().
      Watching in WireShark, the Batch API got a 401, retried properly, then proceeded
      to PUT the blob (without authentication headers).  This got a 401, but the
      client never retried with authentication.  Worse, the blob was sent over the
      wire in the failed attempt.
      
      This kills digests for both the Batch API and the Transfer API.  While in theory
      we could have the Batch API provide external URLs to a place that supports Basic
      Authentication, the LFS spec actually calls out using Basic Authentication[1].
      It's not clear to me if they've been able to shoehorn in other methods.  But
      let's keep it simple until somebody needs it.
      
      If we only had to support python2, we could just not add the handler for digest
      authentication.  However in python3, AbstractBasicAuthHandler raises ValueError
      if it sees a scheme other than Basic.  So we need to intercept all other schemes
      before it gets to that point.
      
      # no-check-commit because of urllib2.OpenerDirector foo_bar calling conventions
      
      [1] https://github.com/git-lfs/git-lfs/blob/master/docs/api/authentication.md
      698667eb7523
  15. Jan 29, 2019
    • Matt Harbison's avatar
      lfs: explicitly add the Content-Length header when uploading blobs, for py3 · 1bc01490178a
      Matt Harbison authored
      This was the reason for test-lfs-test-server.t#git-server complaining about an
      "invalid byte in chunk length".  For some reason if this isn't explicitly added,
      py3.7.1 is adding `transfer-encoding: chunked` as well as `Content-length: x`.
      Wireshark flagged this as malformed.  However, if this is set, it doesn't bother
      with `transfer-encoding`.
      
      Before this patch with py3:
      
        PUT /objects/31cf46fbc4ecd458a0943c5b4881f1f5a6dd36c53d6167d5b69ac45149b38e5b HTTP/1.1
        Accept-Encoding: identity
        Content-length: 12
        accept: application/vnd.git-lfs
        content-type: application/octet-stream
        host: localhost:20062
        transfer-encoding: chunked
        user-agent: git-lfs/2.3.4 (Mercurial 4.9rc0+149-7eb7637e34bf)
      
      Before this patch with py27:
      
        PUT /objects/31cf46fbc4ecd458a0943c5b4881f1f5a6dd36c53d6167d5b69ac45149b38e5b HTTP/1.1
        Accept-Encoding: identity
        accept: application/vnd.git-lfs
        content-type: application/octet-stream
        content-length: 12
        host: localhost:20062
        user-agent: git-lfs/2.3.4 (Mercurial 4.9rc0+149-7eb7637e34bf+20190128)
      
      With this patch and py3, the content is the same as the py27 example.  RFC2616
      says to ignore `Content-Length` if `Transfer-Encoding` is present, so maybe
      there's nothing to do in the hg-server side (though I'm not sure which it is
      using if presented both).
      
      Maybe chunked encoding is better to do?  If someone knows how to suppress the
      `Content-Length`, we can try that instead.
      1bc01490178a
  16. Jan 28, 2019
  17. Jan 27, 2019
  18. Nov 19, 2018
  19. Nov 16, 2018
  20. Nov 15, 2018
    • Matt Harbison's avatar
      lfs: make the exception messages consistent · 93e5d18251d6
      Matt Harbison authored
      I don't love that it repeats 'HTTP Error' in an already long message, but I
      doubt that we should assume that it will always say that on the original
      exception message.
      93e5d18251d6
    • Matt Harbison's avatar
      lfs: handle URLErrors to add additional information · 380f5131ee7b
      Matt Harbison authored
      Sometimes the blob server is hit first (e.g. on push), and sometimes it's hit
      last (e.g. pull).  Throw in depth first subrepo operations, and things quickly
      get insane.  It wasn't even mentioning LFS, so just saying "connection refused"
      can be confusing- especially if the blob server is a secondary server and
      connecting to the repo server works.
      
      The exception handler for the transfer handler will print the full path to the
      blob, but that seems fine given that it might be necessary to debug a second
      server.  (We don't yet support a standalone blob server, so the handler for the
      Batch API will cover 99.9% of the current problems.  But it might as well be
      handled now while I'm thinking about it.)
      
      The function for translating to a message was mostly borrowed from
      scmutil.catchall().
      380f5131ee7b
    • Matt Harbison's avatar
      lfs: improve the hints for common errors in the Batch API · 9f78d10742af
      Matt Harbison authored
      The previous message was too debug-ish and less action oriented than a hint
      should be.  The remaining errors that aren't handled are more along the lines of
      programming errors (not using POST, bad accept type, etc), so I'm not bothering
      with that.
      
      The friendly errors purposely use `self.baseurl` instead of the full Batch API
      endpoint because I'd expect some copy/paste/modify on the part of the user here,
      and it would be more confusing if '/objects/batch' magically appeared, but
      shouldn't be used in the config setting.  It still seems like the right thing
      for debugging in the catchall case.
      9f78d10742af
    • Matt Harbison's avatar
      lfs: provide more Batch API error info via a hint in the raised exception · 8863f08c1630
      Matt Harbison authored
      A coworker had a typo in `lfs.url`, forgot it was even set because usually the
      blob server is inferred, and then got a 404.  It would have been easier to debug
      with the failing URL printed.
      8863f08c1630
  21. Sep 18, 2018
  22. Sep 06, 2018
    • Matt Harbison's avatar
      lfs: ensure the blob is linked to the remote store on skipped uploads · a913d2892e17
      Matt Harbison authored
      I noticed a "missing" blob when pushing two repositories with common blobs to a
      fresh server, and then running `hg verify` as a user different from the one
      running the web server.  When pushing the second repo, several of the blobs
      already existed in the user cache, so the server indicated to the client that it
      doesn't need to upload the blobs.  That's good enough for the web server process
      to serve up in the future.  But a different user has a different cache by
      default, so verify complains that `lfs.url` needs to be set, because it wants to
      fetch the missing blobs.
      
      Aside from that corner case, it's better to keep all of the blobs in the repo
      whenever possible.  Especially since the largefiles wiki says the user cache can
      be deleted at any time to reclaim disk space- users switching over may have the
      same expectations.
      a913d2892e17
  23. Aug 25, 2018
Loading