[Done] 5.73 Multiple HTTPRequest() lead to a crash

Post bugreports for the Mac OSX version here
User avatar
deseven
Enthusiast
Enthusiast
Posts: 318
Joined: Wed Jan 12, 2011 3:48 pm
Location: Serbia
Contact:

[Done] 5.73 Multiple HTTPRequest() lead to a crash

Post by deseven »

This code will crash after a couple of cycles.

Code: Select all

InitNetwork()

Repeat
  request = HTTPRequest(#PB_HTTP_Get,"https://d7.wtf","",#PB_HTTP_Asynchronous)
  Debug "started request 1"
  
  Delay(100)
  
  request2 = HTTPRequest(#PB_HTTP_Get,"https://d7.wtf","",#PB_HTTP_Asynchronous)
  Debug "started request 2"
  
  While HTTPProgress(request) >= 0 And HTTPProgress(request2) >= 0
    Delay(100)
  Wend
  
  FinishHTTP(request)
  FinishHTTP(request2)
ForEver
User avatar
helpy
Enthusiast
Enthusiast
Posts: 550
Joined: Sat Jun 28, 2003 12:01 am

Re: [5.73] Multiple HTTPRequest() lead to a crash

Post by helpy »

I think that the problem is the following line:

Code: Select all

  While HTTPProgress(request) >= 0 And HTTPProgress(request2) >= 0
If ONE of the two requests has been finished/aborted/failed, then the loop will end.
In this case it is possible that, one of the requests ist still in progress.
Finishing an asynchronus request which ist still in progress, is an ivalid way to handle asynchronous requests!

Just change the While loop to:

Code: Select all

  While HTTPProgress(request) >= 0
    Delay(100)
  Wend
  While HTTPProgress(request2) >= 0
    Delay(100)
  Wend


Or change your code to:

Code: Select all

EnableExplicit

InitNetwork()

Procedure.i WaitForRequestAndFinishHTTP(req)
	Protected progress, *buffer
	Repeat
		progress = HTTPProgress(req)
		Select progress
			Case #PB_HTTP_Success
				*buffer = HTTPMemory(req)
				Debug "Request finished (size: " + MemorySize(*buffer) + ")"
				FreeMemory(*buffer)
				FinishHTTP(req)
				Break
				
			Case #PB_HTTP_Failed
				Debug "Request failed"
				FinishHTTP(req)
				Break
				
			Case #PB_HTTP_Aborted
				Debug "Request aborted"
				FinishHTTP(req)
				Break
				
			Default
				Delay(10)
				
		EndSelect
	ForEver
	ProcedureReturn progress
EndProcedure

Define request, request2
Repeat
  request = HTTPRequest(#PB_HTTP_Get,"https://d7.wtf","",#PB_HTTP_Asynchronous)
  Debug "started request 1"
  
  Delay(100)
  
  request2 = HTTPRequest(#PB_HTTP_Get,"https://d7.wtf","",#PB_HTTP_Asynchronous)
  Debug "started request 2"
  
  WaitForRequestAndFinishHTTP(request)
  WaitForRequestAndFinishHTTP(request2)
ForEver
Windows 10 / Windows 7
PB Last Final / Last Beta Testing
User avatar
deseven
Enthusiast
Enthusiast
Posts: 318
Joined: Wed Jan 12, 2011 3:48 pm
Location: Serbia
Contact:

Re: [5.73] Multiple HTTPRequest() lead to a crash

Post by deseven »

You're right, i should've written "Or" instead of "And" there.
However, there's still something fishy here.

First of all, the documentation for FinishHTTP() states that "The value #PB_HTTP_Aborted will be returned by HTTPProgress()."
I don't understand why HTTPProgress() should work here at all, since FinishHTTP() "frees the resources associated to the specified asynchronous download", but let's try following the documentation anyway:

Code: Select all

InitNetwork()

request = HTTPRequest(#PB_HTTP_Get,"https://d7.wtf","",#PB_HTTP_Asynchronous)
FinishHTTP(request)

Delay(500) ; comment this to see the difference

Select HTTPProgress(request)
  Case #PB_HTTP_Success
    Debug "#PB_HTTP_Success"
  Case #PB_HTTP_Failed
    Debug "#PB_HTTP_Failed"
  Case #PB_HTTP_Aborted
    Debug "#PB_HTTP_Aborted"
  Default
    Debug "got no status"
EndSelect
You never actually get #PB_HTTP_Aborted here.

Moving on.
Finishing an asynchronus request which ist still in progress, is an ivalid way to handle asynchronous requests!
Nothing actually states that we should specifically wait for a valid HTTPProgress() to be able to finish the request.
The example above with just one instance of HTTPRequest() won't crash, but this will:

Code: Select all

InitNetwork()

NewList requests.i()

Repeat
  For i = 1 To 100
    AddElement(requests())
    requests() = HTTPRequest(#PB_HTTP_Get,"https://localhost","",#PB_HTTP_Asynchronous) ; replaced to localhost to not DoS my server
    Debug "started request " + Str(i)
  Next
  
  ForEach requests()
    ;AbortHTTP(requests()) ; doesn't help anyway
    FinishHTTP(requests())
    Debug "finished request " + Str(ListIndex(requests()) + 1)
  Next
  
  ClearList(requests())
ForEver
Could be related to the bug with AbortHTTP()? FinishHTTP() also doesn't have any checks and it doesn't wait for an actual result?

At the very least, I believe this should be correctly caught by the debugger and the documentation should explicitly state what should be expected and when.
User avatar
helpy
Enthusiast
Enthusiast
Posts: 550
Joined: Sat Jun 28, 2003 12:01 am

Re: [5.73] Multiple HTTPRequest() lead to a crash

Post by helpy »

I think, the documentation is not correct (or complete) for all cases!

With asynchronous call of ReceiveHTTPFile or ReceiveHTTPMemory FinishHTTP should be used as follows:
... When using #PB_HTTP_Asynchronous, FinishHTTP() needs to be called when the download is finished (successfully or not). ...
This means that you have to use HTTProgress to determine the state of the download and only if Progress is Success, Failed OR Aborted, than you should use HTTPFinish.

But with asynchrous call of HTTPRequest or HTTPRequestMemory the documentation says:
... FinishHTTP() has to be always called to finish a successfully initialized HTTP request, even if the call was synchronous. ...
... this is not clear for me!
(1) with synchronous call a HTTP request only returns non-zero (== sucessfully initizialized) if the download itself was finished otherwise it returns zero!
(2) with asynchronous call a HTTP request retruns non-zero if the download has started succesfully.
Therfore I do not know what the quoted sentence "FinishHTTP() has to be always called to finish a successfully initialized HTTP request, even if the call was synchronous." really mean.

I assume that all four asynchronous HTTP requests (HTTPRequest, HTTPRequestMemory, ReceiveHTTPFile, ReceiveHTTPMemory) share the same code base.
Therfore based on my assumptions for asynchronous HTTP requests I suggest:
  • Always check the return value of all HTTP request calls!
  • Never use FinishHTTP() to abort a running asynchronous HTTP request!
  • Only use FinishHTTP() if HTTPProgress returned #PB_Http_Sucess, #PB_Http_Failed or #PB_Http_Abort.
  • If you need to call HTTPInfo or HTTPMemory, only do this before the call of HTTPFinish.
These rules work for me and I do not get the errors caused by your examples.

@PureBasic team:
A more detailed and complete documetation of HTTP library would be fine!
Windows 10 / Windows 7
PB Last Final / Last Beta Testing
User avatar
deseven
Enthusiast
Enthusiast
Posts: 318
Joined: Wed Jan 12, 2011 3:48 pm
Location: Serbia
Contact:

Re: [5.73] Multiple HTTPRequest() lead to a crash

Post by deseven »

helpy wrote: Sun May 16, 2021 9:53 am Therfore based on my assumptions for asynchronous HTTP requests I suggest:
  • Always check the return value of all HTTP request calls!
  • Never use FinishHTTP() to abort a running asynchronous HTTP request!
  • Only use FinishHTTP() if HTTPProgress returned #PB_Http_Sucess, #PB_Http_Failed or #PB_Http_Abort.
  • If you need to call HTTPInfo or HTTPMemory, only do this before the call of HTTPFinish.
These rules work for me and I do not get the errors caused by your examples.
Yeah, i figured as much, but it took a lot of effort (since the debugger isn't helpful in that case at all) and countless hours of trying to understand what's going on in a pretty complex app. None of it would've happened if the documentation was more clear and those crashes were correctly handled by the debugger.

Thanks for digging into it.
Rinzwind
Enthusiast
Enthusiast
Posts: 536
Joined: Wed Mar 11, 2009 4:06 pm
Location: NL

Re: [5.73] Multiple HTTPRequest() lead to a crash

Post by Rinzwind »

&#^@!@
This behavior of FinishHTTP caused random and difficult to analyse Windows server reboots (cause it runs in a high security context). It should be safe to call in any circumstance and the documentation should be specific.

Seems AbortHTTP does not make HTTPProgress return #PB_HTTP_Aborted right away. Not without a LONG delay (2 seconds?) before checking HTTPProgress. Impractical.

ps. still missing a timeout parameter for the async Request procedures.
User avatar
deseven
Enthusiast
Enthusiast
Posts: 318
Joined: Wed Jan 12, 2011 3:48 pm
Location: Serbia
Contact:

Re: [5.73] Multiple HTTPRequest() lead to a crash

Post by deseven »

I can suggest using a module I wrote which will gracefully handle such situations (among other things). It also has support for timeouts (not on libcurl level, since I wanted to use PB procedures only, but at least you can expect that your code won't be stuck on waiting for HTTP request).

https://github.com/deseven/pb-httprequest-manager
Rinzwind
Enthusiast
Enthusiast
Posts: 536
Joined: Wed Mar 11, 2009 4:06 pm
Location: NL

Re: [5.73] Multiple HTTPRequest() lead to a crash

Post by Rinzwind »

@dseven
Thanks for that alternative. Fred should hire you, then things finally 'happen' 8)
User avatar
deseven
Enthusiast
Enthusiast
Posts: 318
Joined: Wed Jan 12, 2011 3:48 pm
Location: Serbia
Contact:

Re: [5.73] Multiple HTTPRequest() lead to a crash

Post by deseven »

Rinzwind wrote: Fri May 06, 2022 2:30 am @dseven
Thanks for that alternative. Fred should hire you, then things finally 'happen' 8)
I'm extremely far from having enough experience to work on PB, I'm really just trying to cover what I can. But thanks! :)

This struggle with PB HTTP library lasted almost a year for me, I was having crashes in random places with no help from the debugger whatsoever, so I know how it goes. Sorry you had to encounter this yourself.
Fred
Administrator
Administrator
Posts: 14729
Joined: Fri May 17, 2002 4:39 pm
Location: France
Contact:

Re: [5.73] Multiple HTTPRequest() lead to a crash

Post by Fred »

Fixed.
Post Reply