Skip to main content

Public

Project privacy set to public. By default, its content is available to everyone (authenticated or not). Please note that more restrictive permissions might exist on some items.

    •  
      request #13492 Improve TUS implementation to support client unable to use PATCH
    Actions
    Infos
    #13492
    Guilhem Bonnefille (CS) (gbonnefille)
    2019-08-23 08:55
    2019-06-14 22:11
    14600
    Details
    Improve TUS implementation to support client unable to use PATCH
    While trying to upload file from Jenkins Pipeline, I'm faced to an inextricable issue.
    TUS protocol suggest a PATCH to send content. But this method is not supported by OpenJDK: https://bugs.openjdk.java.net/browse/JDK-7016595
    In such situation, Tus Java implementation switches to POST+X-HTTP-Method-Override=PATCH:
    https://github.com/tus/tus-java-client/

    This behavior is quite standard and documented in TUS: https://tus.io/protocols/resumable-upload.html#x-http-method-override

    But it seems the TUS implementation in Tuleap does not support this feature. The POST method is not routed and the TusServer does not check this situation.
    Other PHP implementation explicitly support and test this approach: https://github.com/ankitpokhrel/tus-php

    As Jenkins is the reference CI system, it would be useful to be compliant with this solution to let Java client working.
    Delivery/File release system
    11.2
    Empty
    • [ ] enhancement
    • [ ] internal improvement
    Empty
    Stage
    Thomas Gerbet (tgerbet)
    Closed
    2019-08-23
    Attachments
    Artifact links
    Empty
    Empty
    References
    Referencing request #13492

    Git commit

    tuleap/tuleap/stable

    Merge commit 'refs/changes/61/15261/2' of ssh://gerrit.tuleap.net:29418/tuleap into HEAD 303463c012
    User avatar Nicolas Terray (nterray) , 2019-06-28 17:38
    request #13492: Support overriding request's method with a header when serving Tus requests 36981ab17e
    User avatar Thomas Gerbet (tgerbet) , 2019-06-21 18:44
    Deploy a dedicated PHP FPM pool to deal with long running requests 665f688665
    User avatar Thomas Gerbet (tgerbet) , 2019-07-05 11:53
    Referenced by request #13492

    Other

    gerrit #15261
    gerrit #15379
    Display settings

    Follow-ups

    User avatar
    Manuel Vacelet (vaceletm)2019-07-01 12:01

    We can either create a git repo in the Tuleap project otherwise you can create your own project here on tuleap.net and organise yourself as you wish (on the plus side, your contrib won't be lost amongts all the other git repo of the projet).

    User avatar
    Guilhem Bonnefille (CS) (gbonnefille)2019-07-01 10:55
    Thanks for solving this issue.

    I will check what I can release or not. Is there any "contrib" sub-directory or repository to submit contributions related to Tuleap? I know we also have an other piece of code for Jenkins library in order to submit build status associated to a commit.
    User avatar
    Manuel Vacelet (vaceletm)2019-07-01 09:03

    @gbonnefille Thanks for reporting that, could you please share the code snippet you are using in Jenkins ? I'm pretty sure it could be an handy addition to the doc.

     

    User avatar
    Thomas Gerbet (tgerbet)2019-06-21 18:41
    Support of the X-Http-Method-Override for the requests served with the tus protocol is currently being reviewed here: gerrit #15261.
    • Status changed from Verified to Under review
    • Assigned to changed from None to Thomas Gerbet (tgerbet)
    User avatar
    Guilhem Bonnefille (CS) (gbonnefille)2019-06-17 16:38
    Sorry, I did not understand.
    Reading "Running a production instance with a large value for the memory_limit directive is IMO not suitable at all for production instances" I understood you want to implement a solution supporting HUGE POST actions.

    In the case of TUS, the post_max_size/upload_max_size directives are not related to the chunk but will be associated to the resulting file?
    User avatar
    Thomas Gerbet (tgerbet)2019-06-17 15:39
    Yes, this exactly what the protocol state and my previous follow up explains why the mutation of the HTTP verb according to the X-HTTP-Method-Override header has not been implemented.
    User avatar
    Guilhem Bonnefille (CS) (gbonnefille)2019-06-17 14:32
    Looking deeper in this topic, it seems that the X-HTTP-Method-Override value should be used as the **REAL** method called. The wrapping POST call should be considered as a transport solution only.
    https://www.infoworld.com/article/3249687/how-to-implement-a-delegatinghandler-for-x-http-method-override-in-web-api.html

    This behavior is clearly observable in the TUS implementations:
    - https://github.com/tus/tusd
    - https://github.com/tus/tus-node-server

    If supporting X-HTTP-Method-Override in a POST, you should not implement a single huge upload but really the full TUS protocol, with PATCH wrapped into POST.
    User avatar
    Guilhem Bonnefille (CS) (gbonnefille)2019-06-17 10:18
    As I'm not HTTP expert, I initially imagined that X-HTTP-Method-Override allows to implement a PATCH logic using POST vocabulary. Of course, if you have to implement a true POST method, it would be less usefull, particularly for really large files (I'm trying to upload Docker image dump).

    Are you sure you have to implement a real POST method?
    User avatar
    Thomas Gerbet (tgerbet)2019-06-17 10:08
    Hi,

    Yes the part of the tus protocol about the handling of the X-HTTP-Method-Override header is not respected.
    To be honest this omission was voluntary. Allowing large uploads with POST requests means that we need to have the post_max_size/upload_max_size directives set accordingly and since PHP by default will read the data to populate the $_POST and $_FILES which means having a memory_limit directive sets to a larger value than the post_max_size directive. Running a production instance with a large value for the memory_limit directive is IMO not suitable at all for production instances and we cannot disable the directive enable_post_data_reading because large parts of Tuleap require it.

    This is why the switch with the X-HTTP-Method-Override header is not supported ATM (it's an ugly hack after all). That's said all the routes handling the large file upload are segregated from the others and already have some specific configurations, we might have a way to disable the directive enable_post_data_reading for those routes only.
    • Status changed from New to Verified