TIPClientHttp fails to upload binary files

alencar commented 7 years ago

When using TipClientHttp to upload binary files, it will fail, but works just file for non-binary files.

To reproduce, a server-side upload handle is needed, below, the two sample code to reproduce the bug, the Harbour part and the server part as a PHP simple file upload handle.

Harbour code

Save it as fileupload.prg, run hbmk2 fileupload hbtip.hbc

To execute: (example.com should be changed by your server name/ip address)

$ fileupload fileupload.prg http://example.com/upload.php

Expected result: OK Obtained result: OK

Sample output:

Local file hash (MD5)... 38d4699ee3d0eab60b10e8480a34ac97
Local file size.........       1181
TIP Client HTTP Status
Remote file hash (MD5).: 38d4699ee3d0eab60b10e8480a34ac97
Remote file size.......:       1181
Local and remote sizes are equals? .T.
Local and remote hashs are equals? .T.

$ fileupload fileupload.exe http://example.com/upload.php

Expected result: OK (assuming Windows platform because of binary name) Obtained result: Failed upload, file size differ, hash differ (as consequence)

Sample output:

Local file hash (MD5)... 77983d34d059e4fe0b2ca39487ec04ae
Local file size.........    1943039
TIP Client HTTP Status
Remote file hash (MD5).: 82bef32153657dc25dc439a3b500a614
Remote file size.......:          3
Local and remote sizes are equals? .F.
Local and remote hashs are equals? .F.

fileupload.prg source-code

REQUEST HB_GT_WIN
REQUEST HB_CODEPAGE_PTISO

PROCEDURE Main( cFile, cUrl )

LOCAL oHttp, cResponse, hResponse, cFileHash, nFileSize

IF Empty( cFile ) .OR. Empty ( cUrl )
   ? "Usage FileUpload <cFileName> <cUrl>"
   QUIT
ENDIF

cFileHash := hb_MD5File( cFile ) // Just for simplicity, avoid MD5
? "Local file hash (MD5)...", cFileHash
nFileSize := hb_FSize( cFile )
? "Local file size.........", nFileSize

oHttp := TIPClientHTTP():New( cUrl, .T. )

IF oHttp:open()
    oHttp:Attach("files", cFile, "application/octet-stream")
    oHttp:PostMultiPart()

    ? "TIP Client HTTP Status", oHttp:lastErrorMessage()

    cResponse := oHttp:ReadAll()

    IF .NOT. Empty( cResponse )
        hb_jsonDecode(cResponse, @hResponse)
        ? "Remote file hash (MD5).:", hResponse["filehash"]
        ? "Remote file size.......:", hResponse["filesize"]

        ? "Local and remote sizes are equals?", nFileSize == hResponse["filesize"]
        ? "Local and remote hashs are equals?", cFileHash == hResponse["filehash"]
        ? ""
    ENDIF

    INKEY(0)
ELSE
    ? "Error:", oHttp:lastErrorMessage()
ENDIF

oHttp:close()

Server Code (PHP/Apache)

Just save this piece of code as upload.php inside any accessible server directory.

upload.php source-code

<?php

error_reporting(E_ALL);

if(isset($_FILES))
{
    if($_FILES["files"]["error"] > 0)
    {
        echo "Error: " . $_FILES["files"]["error"] . "<br/>";
    }
    else
    {
        echo json_encode(array("filename" => $_FILES["files"]["name"], "filetype" => $_FILES["files"]["type"], "filesize" => $_FILES["files"]["size"], "filehash" => md5_file($_FILES["files"]["tmp_name"])));

    }
}

?>

vszakats commented 7 years ago

Is this the same problem you reported a few years ago?: https://groups.google.com/d/msg/harbour-users/wOsubO8oifk/Jb9L8ZYAyHgJ

vszakats commented 7 years ago

Looks like it. It was fixed in 3.4 in this commit: https://github.com/vszakats/harbour-core/commit/09178e4422206bc23c67c7f017c7a1616650811a

alencar commented 7 years ago

@vszakats exactly this one.

xHarbour r10193 stills bugged Harbour main-line stills bugged

Cannot test on vszakats/harbour-core because of https://github.com/harbour/core/issues/143

C:\temp\vszakats\harbour-core>git pull
Updating 832f7ab486..f27c560ad4
....
C:\temp\vszakats\harbour-core>mingw32-make
! Building Harbour 3.4.0dev from source
! MAKE: mingw32-make 4.1 'sh.exe'
! HB_INSTALL_PREFIX: C:\HARBOUR
! HB_BUILD_STRIP: ALL
! HB_HOST_PLAT: win (x86_64)  HB_SHELL: nt
! HB_PLATFORM: win (x86) (auto-detected)
! HB_COMPILER: mingw (v0603) (auto-detected: C:/MinGW-6.3.0/bin/)
! Component: 'zlib' found in C:/temp/vszakats/harbour-core/src/3rd/zlib (local)
! Component: 'pcre2' not found. Configure with HB_WITH_PCRE2.
! Component: 'pcre' found in C:/temp/vszakats/harbour-core/src/3rd/pcre (local)
! Component: 'gpm' not supported on win platform
! Component: 's-lang' not found. Configure with HB_WITH_SLANG.
! Component: 'curses' not found. Configure with HB_WITH_CURSES.
! Component: 'x11' not found. Configure with HB_WITH_X11.
! Component: 'wattcp/watt-32' not supported on win platform
! WARNING: Git not found in PATH. Version information might not be accurate.
mingw32-make[1]: Nothing to be done for 'all'.
mingw32-make[1]: Nothing to be done for 'all'.
! 'hbdossrl' library skipped (platform or compiler not supported)
mingw32-make[4]: '../../../../../../lib/win/mingw/libpng.a' is up to date.
mingw32-make[4]: '../../../../../../lib/win/mingw/libhbpcre.a' is up to date.
! 'hbpcre2' library skipped (local copy unused)
mingw32-make[4]: '../../../../../../lib/win/mingw/libhbzlib.a' is up to date.
mingw32-make[3]: '../../../../../lib/win/mingw/libhbcommon.a' is up to date.
mingw32-make[3]: '../../../../../lib/win/mingw/libhbnortl.a' is up to date.
mingw32-make[3]: '../../../../../lib/win/mingw/libhbpp.a' is up to date.
mingw32-make[3]: '../../../../../lib/win/mingw/libhbcplr.a' is up to date.
mingw32-make[3]: '../../../../../bin/win/mingw/harbour.exe' is up to date.
mingw32-make[3]: '../../../../../lib/win/mingw/libhbvm.a' is up to date.
mingw32-make[4]: '../../../../../../lib/win/mingw/libhbmainstd.a' is up to date.
mingw32-make[4]: '../../../../../../lib/win/mingw/libhbmainwin.a' is up to date.
mingw32-make[4]: '../../../../../../lib/win/mingw/libhbmaindllh.a' is up to date.
mingw32-make[4]: '../../../../../../lib/win/mingw/libhbmaindllp.a' is up to date.
mingw32-make[4]: '../../../../../../lib/win/mingw/libhbvmmt.a' is up to date.
../../../../../bin/win/mingw/harbour.exe -i../../../../../include -n1 -q0 -w3 -es2 -kmo -i- -l -gc3 ../../../pushbtn.prg
../../../pushbtn.prg(139) Warning W0027  Meaningless use of expression 'Numeric'

No code generated.
../../../../../config/rules.mk:157: recipe for target 'pushbtn.c' failed
mingw32-make[3]: *** [pushbtn.c] Error 1
../../config/lib.mk:72: recipe for target 'descend' failed
mingw32-make[2]: *** [descend] Error 2
../config/dir.mk:71: recipe for target 'rtl' failed
mingw32-make[1]: *** [rtl] Error 2
config/dir.mk:71: recipe for target 'src' failed
mingw32-make: *** [src] Error 2

C:\temp\vszakats\harbour-core>

vszakats commented 7 years ago

That's quite a strange error. I'm actually using Harbour on Sierra and it's working without problems. It's also build daily on Sierra/Windows/Linux CI machines without problem. Not sure what may cause such effect as you're seeing. Looks like your Harbour compiler is completely botched.

I'd suggest starting over with a freshly downloaded source tarball unzipped into a fresh directory and removing any environment variables starting with HB_. Except maybe a HB_BUILD_VERBOSE=yes setting.

alencar commented 7 years ago

@vszakats I have dropped the full source tree of your branch and clonned it again and the build was sucessful (Windows and OS X).

Building the sample on this issue (on Windows), I got an odd behavior. The methods :Attach and :PostMultiPart don't send the attached files to the server, no binary, no plain text.

Can you try it on our side?

vszakats commented 7 years ago

Glad that the builds work now. It also means #143 may now be closed.

It's unclear if you've made this test using 3.2 or 3.4. But, due the endless problems with it, I've personally stopped using hbtip in favour of hbcurl, and consequently hbtip is deprecated in 3.4, so I won't be able to be much of a help here. On the bright side the whole code is pure .prg so it should be relatively easy to debug it yourself. I'm accepting PRs in 3.4 if it addresses a regression compared to 3.2. If this is not feasible, I recommend migrating to hbcurl.

alencar commented 7 years ago

@vszakats It was on 3.4.

I have changed source\tip\httpcln.prg from xHarbour to look like this patch below and it was fixes (very slow because of the small buffer size, but fixed). I am willing to bring it to 3.2 and 3.4 (if you like), but change the buffer size. Does Harbour expose BUFSIZ or some similar constant for PRG-level?

C:\svn.xharbour.org\trunk\xharbour>svn diff
Index: source/tip/httpcln.prg
===================================================================
--- source/tip/httpcln.prg      (revision 10193)
+++ source/tip/httpcln.prg      (working copy)
@@ -54,6 +54,7 @@
 #include "hbclass.ch"
 #include "tip.ch"
 #include "common.ch"
+#include "fileio.ch"
 /**
 * Inet service manager: http
 */
@@ -641,20 +642,23 @@
                '"' + cCrlf + 'Content-Type: ' + cType + cCrLf + cCrLf
       // hope this is not a big file....
       nFile := FOpen( cFile )
-      nbuf  := 8192
-      nRead := nBuf
-      // cBuf := Space( nBuf )
-      WHILE nRead == nBuf
-         // nRead := FRead( nFile, @cBuf, nBuf )
-         cBuf  := FReadStr( nFile, nBuf )
-         nRead := Len( cBuf )
-/*       IF nRead < nBuf
-            cBuf := Pad(cBuf, nRead)
-         ENDIF
-*/
-         cData += cBuf
-      end
-      FClose( nFile )
+      nBuf  := 8192
+      cBuf := Space( nBuf )
+      WHILE ! HB_F_Eof( nFile )
+          nRead := FRead( nFile, @cBuf, nBuf )
+          IF nRead == nBuf
+              cData += cBuf
+          ELSE
+              FSeek( nFile, nRead * (-1), FS_RELATIVE)
+              cBuf := Space( nRead )
+              nBuf := nRead
+              nRead := FRead (nFile, @cBuf, nBuf)
+              IF nRead == nBuf
+                  cData += cBuf
+              ENDIF
+          ENDIF
+      END
+         FClose( nFile )
       cData += cCrlf
    NEXT
    cData += cBound + '--' + cCrlf

vszakats commented 7 years ago

I'm interested in patching 3.4 (not 3.2, sorry) to avoid any regressions the already implemented fixes (including a different version of the above) may have caused in it. If you can provide a patch for the "no upload" problem, you're welcome to open an Issue/PR in the 3.4 repo.

alencar commented 7 years ago

Started porting/testing the patch on @harbour/core. As soon as I fix it there, I will try on yours. But for what I have tested already, the problem is beyond file read, since you changed it to read using the correct way.

alencar commented 7 years ago

@vszakats applied this patch to main-line harbour (3.2) https://github.com/harbour/core and it worked just fine (tested on Mac OS X Sierra and builtin PHP server). Now I will look where 3.4 is breaking and try to fix it.

git diff -p output

diff --git a/contrib/hbtip/httpcli.prg b/contrib/hbtip/httpcli.prg
index ab43547..4f1da47 100644
--- a/contrib/hbtip/httpcli.prg
+++ b/contrib/hbtip/httpcli.prg
@@ -577,20 +577,22 @@ METHOD PostMultiPart( xPostData, cQuery ) CLASS TIPClientHTTP
       // hope this is not a big file....
       nFile := FOpen( cFile )
       /* TOFIX: Error checking on nFile. [vszakats] */
-      nbuf := 8192
-      nRead := nBuf
-      // cBuf := Space( nBuf )
-      DO WHILE nRead == nBuf
-         // nRead := FRead( nFile, @cBuf, nBuf )
-         cBuf := FReadStr( nFile, nBuf )
-         nRead := hb_BLen( cBuf )
-#if 0
-         IF nRead < nBuf
-            cBuf := PadR( cBuf, nRead )
-         ENDIF
-#endif
-         cData += cBuf
-      ENDDO
+      nBuf  := 65536
+      cBuf := Space( nBuf )
+      WHILE ! HB_FEof( nFile )
+          nRead := FRead( nFile, @cBuf, nBuf )
+          IF nRead == nBuf
+              cData += cBuf
+          ELSE
+              FSeek( nFile, nRead * (-1), FS_RELATIVE)
+              cBuf := Space( nRead )
+              nBuf := nRead
+              nRead := FRead (nFile, @cBuf, nBuf)
+              IF nRead == nBuf
+                  cData += cBuf
+              ENDIF
+          ENDIF
+      END
       FClose( nFile )
       cData += cCrlf
    NEXT

alencar commented 7 years ago

Just published a 3.2 updated fork https://github.com/alencar/harbour-core.git with this fix

alencar commented 7 years ago

@vszakats 3.4 PR is up there https://github.com/vszakats/harbour-core/pull/280. When you have some spare time, please take a look, is a simple one line patch.

alcz commented 7 years ago

Thank you for extensive report about this issue, test code and suggested patch. I've commited a fix 11d3cbfa0b94ca90ba4b55e71ca935385bb46639 based on this, although the patch itself is simplified. Please retest.

It should be noted that hbtip contrib library in current state is not perfect for uploading large files, as it is unnecessarily buffering whole file in memory.

vszakats commented 7 years ago

Even the hb_FEof() call can be optimized out by comparing nRead with zero. See code in 3.4.

alcz commented 7 years ago

Yes, I thought about using hb_BLeft() all the time. It's nonetheless interesting that this library seems to be only place in harbour repo that uses hb_FEof()...

I think, if someone finds this library useful for his cases, then it should much more optimized by not reading whole file to the variable. Such rework would make this routine worth updating.

vszakats commented 7 years ago

hb_FEof() is an expensive and redundant function. It was probably added because Eof() is in fact useful when processing DBF tables and some felt it useful to use the similar logic for line/raw file processing, even though it makes these less efficient.

Not sure what you mean be "reading whole file". AFAICS the file is read by 64K chunks, never as a whole.

alcz commented 7 years ago

https://github.com/vszakats/harbour-core/blob/master/contrib/hbtip/httpcli.prg#L553-L574 As I see, the logic is not changed in a fork of yours. Whole file is in cData variable, inetSendAll sends is to the network much later. While we're reading small file chunks, it'd be nice to send them at the same time.

In fact not only one file, but all files supplied in array inside xPostData would end up there!

As for hb_FEof() it additionally conflicts with hbmisc contrib.

vszakats commented 7 years ago

Oh okay, so you meant forming the output in memory. No, it isn't changed indeed, but in fact all communications-related hbtip logic is deprecated in 3.4 in favour of hbcurl. hbtip has plenty of other problems.

The conflict with hbmisc was resolved in 3.4 by enabling deprecations and redirecting the whole API family to the original functions in NFLIB. (the hbmisc hb_F*() API was a limited, copy-paste version of the ft_F*() API in hbnf.)

alencar commented 7 years ago

@alcz thanks for you attention, I have made some changes based on @vszakats code. If you like it, I can push it to my fork or you can get it right below. I have get rid of hb_FEof(), but the read all into the memory and then sends stills there.

diff --git a/contrib/hbtip/httpcli.prg b/contrib/hbtip/httpcli.prg
index ab4354783e..a2353c1dc2 100644
--- a/contrib/hbtip/httpcli.prg
+++ b/contrib/hbtip/httpcli.prg
@@ -533,7 +533,7 @@ METHOD PostMultiPart( xPostData, cQuery ) CLASS TIPClientHTTP
    LOCAL cCrlf := ::cCRlf, oSub
    LOCAL nPos
    LOCAL cFilePath, cName, cFile, cType
-   LOCAL nFile, cBuf, nBuf, nRead
+   LOCAL hFile, cBuffer, nRead

    IF Empty( xPostData )
    ELSEIF HB_ISHASH( xPostData )
@@ -574,27 +574,20 @@ METHOD PostMultiPart( xPostData, cQuery ) CLASS TIPClientHTTP
          cType := "text/html"
       ENDIF
       cData += cBound + cCrlf + 'Content-Disposition: form-data; name="' + cName + '"; filename="' + cTmp + '"' + cCrlf + 'Content-Type: ' + cType + cCrLf + cCrLf
-      // hope this is not a big file....
-      nFile := FOpen( cFile )
-      /* TOFIX: Error checking on nFile. [vszakats] */
-      nbuf := 8192
-      nRead := nBuf
-      // cBuf := Space( nBuf )
-      DO WHILE nRead == nBuf
-         // nRead := FRead( nFile, @cBuf, nBuf )
-         cBuf := FReadStr( nFile, nBuf )
-         nRead := hb_BLen( cBuf )
-#if 0
-         IF nRead < nBuf
-            cBuf := PadR( cBuf, nRead )
-         ENDIF
-#endif
-         cData += cBuf
-      ENDDO
-      FClose( nFile )
+
+      IF ( hFile := hb_vfOpen( cFile, FO_READ ) ) != NIL
+         cBuffer := Space( 65536 )
+         DO WHILE ( nRead := hb_vfRead( hFile, @cBuffer, hb_Blen( cBuffer ) ) ) > 0
+            cData += hb_BLeft( cBuffer, nRead )
+         ENDDO
+         hb_vfClose( hFile )
+      ENDIF
+
       cData += cCrlf
    NEXT
+
    cData += cBound + "--" + cCrlf
+
    IF ! HB_ISSTRING( cQuery )
       cQuery := ::oUrl:BuildQuery()
    ENDIF
@@ -603,7 +596,7 @@ METHOD PostMultiPart( xPostData, cQuery ) CLASS TIPClientHTTP
    ::StandardFields()

    IF ! "Content-Type" $ ::hFields
-      ::inetSendAll( ::SocketCon, e"Content-Type: multipart/form-data; boundary=" + ::boundary( 2 ) + ::cCrlf )
+      ::inetSendAll( ::SocketCon, "Content-Type: multipart/form-data; boundary=" + ::boundary( 2 ) + ::cCrlf )
    ENDIF

    ::inetSendAll( ::SocketCon, "Content-Length: " + hb_ntos( Len( cData ) ) + ::cCRLF )

alcz commented 7 years ago

I've made a compilation of my suggestion to not buffer whole file and the updated code of :PostMultiPart() from Viktor's repository. File was not encoded at all, but byte-copied, therefore i'd prefer such way of doing it. Here I also reused a method from TIPClient base class for writing file to the socket.

https://github.com/alcz/harbour/commit/2dc36f2aee03714c288528238e1596c7b2a11360

Please make some tests.

alencar commented 7 years ago

@alcz I am downloading your fork and will repeat the tests today. I will get back ASAP.

alencar commented 7 years ago

@alcz here are the results:

PS C:\temp\sandbox\harbour> Measure-Command { .\FileUploadAlcz.exe dummy.txt http://127.0.0.1:8080/upload.php }

Local file hash (MD5)... d3d7b9ef0260c43cb06eeb026ed74cc3
Local file size.........        371
TIP Client HTTP Status
Remote file hash (MD5).: d3d7b9ef0260c43cb06eeb026ed74cc3
Remote file size.......:        371
Local and remote sizes are equals? .T.
Local and remote hashs are equals? .T.

TotalSeconds      : 0,3729495

PS C:\temp\sandbox\harbour> Measure-Command { .\FileUpload.exe dummy.txt http://127.0.0.1:8080/upload.php }

Local file hash (MD5)... d3d7b9ef0260c43cb06eeb026ed74cc3
Local file size.........        371
TIP Client HTTP Status
Remote file hash (MD5).: d3d7b9ef0260c43cb06eeb026ed74cc3
Remote file size.......:        371
Local and remote sizes are equals? .T.
Local and remote hashs are equals? .T.

TotalSeconds      : 0,3747199

PS C:\temp\sandbox\harbour> Measure-Command { .\FileUploadAlcz.exe dummy.exe http://127.0.0.1:8080/upload.php }

Local file hash (MD5)... 4a7b60c3525e6b3868ac39c88cf68acf
Local file size.........    6348288
TIP Client HTTP Status
Remote file hash (MD5).: 4a7b60c3525e6b3868ac39c88cf68acf
Remote file size.......:    6348288
Local and remote sizes are equals? .T.
Local and remote hashs are equals? .T.

TotalSeconds      : 0,5193077

PS C:\temp\sandbox\harbour> Measure-Command { .\FileUpload.exe dummy.exe http://127.0.0.1:8080/upload.php }

Local file hash (MD5)... 4a7b60c3525e6b3868ac39c88cf68acf
Local file size.........    6348288
TIP Client HTTP Status
Remote file hash (MD5).: 4a7b60c3525e6b3868ac39c88cf68acf
Remote file size.......:    6348288
Local and remote sizes are equals? .T.
Local and remote hashs are equals? .T.

TotalSeconds      : 0,4841403

PS C:\temp\sandbox\harbour> Measure-Command { .\FileUploadAlcz.exe dummy.7z http://127.0.0.1:8080/upload.php }

Local file hash (MD5)... 60dc076c29a8866f7a18e3f2213ce245
Local file size.........  133812604
TIP Client HTTP Status
Remote file hash (MD5).: 60dc076c29a8866f7a18e3f2213ce245
Remote file size.......:  133812604
Local and remote sizes are equals? .T.
Local and remote hashs are equals? .T.

TotalSeconds      : 3,4713619

PS C:\temp\sandbox\harbour> Measure-Command { .\FileUpload.exe dummy.7z http://127.0.0.1:8080/upload.php }

Local file hash (MD5)... 60dc076c29a8866f7a18e3f2213ce245
Local file size.........  133812604
TIP Client HTTP Status
Remote file hash (MD5).: 60dc076c29a8866f7a18e3f2213ce245
Remote file size.......:  133812604
Local and remote sizes are equals? .T.
Local and remote hashs are equals? .T.

TotalSeconds      : 2,8201264

alcz commented 7 years ago

Thank you very much for testing.

OK, so it transfers bigger files correctly with a little performance penalty. For me it's fine. Now using Harbour and hbtip, I could for example upload something larger from low on memory OpenWRT router or sth similar.

:WriteFromFile() may be changed to use some VF api, not only for reading.

It can be made faster by making C code doing the "copy" operation, similarly to what hb_vfFromSocket(), hb_vfCopyFile() are doing. But i think it's not worth to rewrite portions of hbtip in C, but rather use hbcurl like Viktor says.

Another thing is, that :PostMultiPart() has apparently some functionality for posting form-fields. Is there any test code for this part, to see if it hasn't been broken?

alencar commented 7 years ago

Hi,

Only one of the three tests your code was a little slow. The exe by you changeset was named FileUploadAlcz.exe.

Not in this sample, but I can make one tomorrow and update here. On Mon, 10 Apr 2017 at 15:58 alcz notifications@github.com wrote:

Thank you very much for testing.

OK, so it transfers bigger files correctly with a little performance penalty. For me it's fine. Now using Harbour and hbtip, I could for example upload something larger from low on memory OpenWRT router or sth similar.

:WriteFromFile() may be changed to use some VF api, not only for reading.

It can be made faster by making C code doing the "copy" operation, similarly to what hb_vfFromSocket(), hb_vfCopyFile() are doing. But i think it's not worth to rewrite portions of hbtip in C, but rather use hbcurl like Viktor says.

Another thing is, that :PostMultiPart() has apparently some functionality for posting form-fields. Is there any test code for this part, to see if it hasn't been broken?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/harbour/core/issues/144#issuecomment-293045835, or mute the thread https://github.com/notifications/unsubscribe-auth/ABqNkAPyWgGsqFmkuSU03atr04x18Nvbks5runvLgaJpZM4MbPyy .

alencar commented 7 years ago

@alcz I have tested and form data are transmitted and received correctly. I see no regression in this matter.

If someone want to check, please change:

The fileupload.prg sample from

oHttp:PostMultiPart()

To

oHttp:PostMultiPart(<xParameters>)

Example:

oHttp:PostMultiPart({ "name" => "John", surname => "Doe", "age" => 45, "height" => 1.72, "bio" => "Neque porro quisquam est qui dolorem ipsum quia dolor sit amet, consectetur, adipisci velit..." })

The upload.php sample to add

echo json_encode($_POST);

From a debug session, look at the cResponse or hResponse variables.

Anything else to add/check @vszakats ?

vszakats commented 7 years ago

My only suggestion is try using the existing 3.4 code as a reference (or as-is) as much as possible, it'd make little sense to create two different solutions for the exact same issues. Converting the loop to C is IMO unnecessary as it's unlikely to be the bottleneck in a TCP/IP context (and yep, libcurl does that already in a better way than hbtip could ever realistically achieve.) Same for the output buffering: it's probably unnecessary to optimise for >100MiB/1GiB file sizes for most "typical" Harbour business apps.

alcz commented 7 years ago

I've prepared fork 3.4 hbtip for inclusion in 3.2 tree, only change is that one regarding :PostMultiPart() https://github.com/alcz/harbour/commit/f19a3bc4a12ecb00cbb7d9bfe0856cf37a407e32 Anything more to look at before commiting? @vszakats - i've did reduced version of hbtip/WARNING.txt, but i won't commit, still can send it to you for consideration.

vszakats commented 7 years ago

@alcz Looks good to me, two (+1) comments:

The casing of legacy items restored in hbtip.hbx were lost compared to original 3.2. It'd be best to copy these lines over the current ones from a 3.2 copy. I mean these:
```
DYNAMIC ANSITOHTML
DYNAMIC HB_MAILASSEMBLE
DYNAMIC HB_SENDMAIL
DYNAMIC HTMLTOANSI
DYNAMIC HTMLTOOEM
DYNAMIC OEMTOHTML
```
As for the the PostMultiPart() updates, probably it'd be better to do it in a separate commit, but I'm still not convinced why this is important given typical usage. Using two passes and hb_vfSize() here looks problematic and it creates unnecessary failure modes and prevents reading from a stream.
As for WARNING.txt, please paste the updated version here to see. (I had some pending updates from yesterday, now committed.)

alcz commented 7 years ago

@vszakats It's stripped down version comparing to the one of yours. I think something along this lines may be put in 3.2 tree to discourage hbtip usage.

--- WARNING.txt.vsz 2017-04-13 09:01:23.000000000 +0200
+++ WARNING.txt 2017-04-13 09:37:16.000000000 +0200
@@ -10,7 +10,7 @@
 solution.

 The only reason hbtip hasn't been deleted altogether is compatibility
-with 3.2, xHarbour and existing projects, plus the fact there are still some
+with xHarbour and existing projects, plus the fact there are still some
 low-level functions that do work fine (tip_MimeType()) or have no better
 replacement yet (tip_MailAssemble()).

@@ -30,17 +30,9 @@
 command-lines can also be converted into compilable/adaptable API code using
 the `--libcurl <targe-source-file>` curl option.

-3.4 snapshot binaries for Windows come bundled with all required static and
-dynamic libraries to use hbcurl, including OpenSSL and libssh2, both 32 and
-64-bit. See this page about feature support and other details:
-
-   https://github.com/vszakats/harbour-deps/
-
-
 If you opt to use hbtip anyway, and find any problem with it (like very common
 SMTP or FTP incompatibilities with various live servers found on the internet),
-your best bet is to report these on mainline 3.2 in the hope someone will
-address it, or even better to address the problem yourself. As for 3.4, I'm
-interesed only in reports that address any regression in 3.4 compared to 3.2.
+your best bet is to address the problem yourself. Patches of sufficent quality
+will be of course accepted.

 -Viktor

I'm not forcing the change in PostMultiPart(), but as you can't imagine uploading large (comparing to system memory available) files using hbtip, I can't imagine that whoever would want to upload an stream/unfinished file as form field and HTTP requires that the Content-Length is correct. Of course the previous buffered approach will be correct by HTTP protocol requirements, because it'd read all the available data and later calculate Content-Length.

I'm on the other hand exploiting Harbour portability and use it according to project needs on various embedded platforms with the memory as low as 16/32MB. Therefore I imagine POSTing data from some embedded data logger using hbtip, where it's even better that Harbour provides everything and curl dependency can be skipped optionally - according to the needs of course.

If we can't tell what's better, then maybe we can write down the second option as PostMultiPartLarge() or it can just sit in my repository if there is no interest from others.

vszakats commented 7 years ago

Thanks for the WARNINGS.txt update. I think hbtip is not just for compatibility with xHarbour but also with previous Harbour stable versions, so maybe the easiest is to swap 3.2 with 3.0 there. As for the rest I don't mind anything as long it's fine with 3.2 users/developers, anyhow I'd prefer to remove my signature, to avoid misunderstandings that I'm trying to define policy for 3.2.

As for the in-memory upload: I understand there exist platforms where memory pressure is high, but I'd argue that f.e. embedded platforms are an atypical Harbour environment, and it's disputable if such environment should use hbtip in the first place for best memory efficiency (f.e. compared to the built-in communications protocol stack/library), and if optimising this single method for very large files is indeed the top priority on these platforms. I'd also argue that on these platforms it'd be better to use a purely C communications library like libcurl.

Ultimately the problem is that the current implementation does come with some drawbacks that all other platforms would have to deal with. It may even mean that so far working apps would randomly break after this modification (f.e. when attempting to upload files actively being written to by other processes.)

I wouldn't mind a solution that doesn't suffer from the current drawback. Not sure if the protocol allows this, but complex questions like this is why I prefer to use f.e. libcurl where these problems are most likely solved adequately. (Interestingly I had to open a PUT-related Issue for libcurl just yesterday, where it sometimes fails when using HTTP/2.)

vszakats commented 7 years ago

One more minor note, I see you removed links to harbour-deps. It's fine with me, but harbour-deps is perfectly usable with 3.2 as well, and the binaries are in fact listed on the curl download page amongst the universal binary builds. It's a frequent question on forums where to get libcurl, so maybe it'd be useful to leave a pointer there.

alcz commented 7 years ago

Thanks for all the notes.

Now commited the import of 3.4 to the 3.2 core without the :PostMultiPart() change. As we discuss, the cases are extending further... To support posting streams, hbtip should should implement "Transfer-Encoding: chunked" where "Content-Length" isn't needed. I'm not interested in adapting the code, bearing in mind the deprecation hints on "hbtip".

Unless someone is using non-standard HTTP server with hbtip, which is not enforcing "Content-Length", streaming with current code won't work either, even if we patch the loop to have some timeout when 0 bytes are read.

harbour / core

TIPClientHttp fails to upload binary files #144

Harbour code

Server Code (PHP/Apache)