4

Here's a routine to do an HTTP request using WinINet and either return the fetched string or raise an exception:

function Request(const pConnection: HINTERNET; const localpath: string): string;
var Buffer: packed Array[1..5000] of Char; BytesRead: Cardinal; pRequest: HINTERNET;     sent: boolean;
begin
Result := '';
pRequest := HTTPOpenRequest(pConnection, 'GET', pchar(localpath), nil, nil, nil, 0, 0);
if pRequest <> nil then
  begin
  sent := HTTPSendRequest(pRequest, nil, 0, nil, 0);
  if sent then
    while InternetReadFile(pRequest, @Buffer, SizeOf(Buffer)-1 {leave room for terminator}, BytesRead) do
      begin
      Buffer[BytesRead+1] := #0;
      Result := Result + buffer;
      end;
  InternetCloseHandle(pRequest);
  if not sent then RaiseLastOSerror; // HTTPSendRequest failed
  end
else RaiseLastOSerror; // HTTPOpenRequest failed
end;

If InternetCloseHandle(pRequest) can fail even though pRequest was successfully assigned, GetLastError() will return an error code for InternetCloseHandle() instead of HTTPSendRequest(). Fixing that would require code like:

function Request(const pConnection: HINTERNET; const localpath: string): string;
var Buffer: packed Array[1..5000] of Char; BytesRead: Cardinal; pRequest: HINTERNET;
begin
Result := '';
pRequest := HTTPOpenRequest(pConnection, 'GET', pchar(localpath), nil, nil, nil, 0, 0);
if pRequest <> nil then
  begin
  if HTTPSendRequest(pRequest, nil, 0, nil, 0) then
    while InternetReadFile(pRequest, @Buffer, SizeOf(Buffer)-1 {leave room for terminator}, BytesRead) do
      begin
      Buffer[BytesRead+1] := #0;
      Result := Result + buffer;
      end
  else
    begin
    InternetCloseHandle(pRequest);
    RaiseLastOSerror; // HTTPSendRequest failed
    end;
  InternetCloseHandle(pRequest);
  end
else RaiseLastOSerror; // HTTPOpenRequest failed
end;

but that seems a lot uglier and more confusing at first glance.

Is it safe to assume InternetCloseHandle() won't fail, thereby allowing the simpler code?

Witness Protection ID 44583292
  • 1,072
  • 1
  • 8
  • 25

4 Answers4

4

First of all this kind of code is not helpful:

raise Exception.Create(IntToStr(GetLastError))

Use this instead:

RaiseLastOsError; // This raises an exception with the description of the error

Since you're using exceptions in your code, how about calling the function like this so it raises an exception if the handle can't be closed?

Win32Check(InternetCloseHandle(H))
Cosmin Prund
  • 25,498
  • 2
  • 60
  • 104
  • 4
    Much better just to call `Win32Check(InternetCloseHandle(H))` – David Heffernan Jul 05 '11 at 14:09
  • Doesn't answer the question. I don't care if the handle can be closed, I want to raise the Send exception without picking up the error code from attempting to close the handle. – Witness Protection ID 44583292 Jul 05 '11 at 14:22
  • @mike: Why not do `Win32Check` or `RaiseLastOsError` (both of which rely on `GetLastError`) as soon as possible? If you do like you should, that is, if you do like I suggest in my answer, the `InternetCloseHandle` will be run even if an exception occurrs. – Andreas Rejbrand Jul 05 '11 at 14:25
2

I think you are going about this the wrong way. You should simply check for errors on each API call and raise an exception as soon as you encounter one. That way you get the error message appropriate to the error that produces the exception. You simply can't expect to carry on calling other API functions and then raise an exception for an error that happened some time ago.

I think you want something along these lines:

Result := '';
pRequest := HTTPOpenRequest(pConnection, 'GET', pchar(localpath), nil, nil, nil, 0, 0);
if pRequest=nil then
  RaiseLastOSerror;
try
  CheckWin32Error(HTTPSendRequest(pRequest, nil, 0, nil, 0));
  while InternetReadFile(pRequest, @Buffer, SizeOf(Buffer)-1, BytesRead) do begin
    Buffer[BytesRead+1] := #0;
    Result := Result + buffer;
  end;
  if GetLastError<>0 then
    RaiseLastOSerror;
finally
  CheckWin32Error(InternetCloseHandle(pRequest));
end;

Note that you didn't include any error checking for InternetReadFile. I've attempted to write it for you.

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
1

You need to make sure that a handle is 'closed' if it is successfully created. That is, you should do

hReq := HTTPOpenRequest(...);
if hReq <> 0 then
  try      
    // Do stuff
  finally
    InternetCloseHandle(hReq);
  end;
Andreas Rejbrand
  • 105,602
  • 8
  • 282
  • 384
  • Doesn't answer the question. The code already closes the handle. The question is whether that can fail before the desired exception is raised. – Witness Protection ID 44583292 Jul 05 '11 at 14:20
  • 1
    @mike: No, you are wrong. What if `HTTPSendRequest` or `InternetReadFile` raises an exception? Then the execution will stop without calling `InternetCloseHandle`. This is the entire point of the `try..finally..` construct in Delphi! – Andreas Rejbrand Jul 05 '11 at 14:23
  • @mike: Why not do Win32Check or RaiseLastOsError (both of which rely on GetLastError) as soon as possible? If you do like you should, that is, if you do like I suggest in my answer, the InternetCloseHandle will be run even if an exception occurrs. – Andreas Rejbrand Jul 05 '11 at 14:26
  • Isn't it true HTTPSendRequest and InternetReadFile don't generate exceptions, they return false or nil to indicate failure? – Witness Protection ID 44583292 Jul 05 '11 at 14:28
  • @mike, according to [the documentation](http://msdn.microsoft.com/en-us/library/aa384350(v=vs.85).aspx) the function has a return value that shows failure and invites you to call `GetLastError` if that happens. There you have it, this literally answers your question. But that's true for virtually all API calls, the art is in handling those errors. – Cosmin Prund Jul 05 '11 at 14:30
  • @mike: No, there is no guarantee that they don't raise any exceptions. In particular, if there is something wrong with the buffer you provided, an exception will be raised. In addition, the `try..finally` appraoch is the normal one. A benefit of this is that you close your handle in the `finally` block, which is guaranteed to run even if an exception occurrs. If you are not inside a `try..finally`, execution just stops at an exception. – Andreas Rejbrand Jul 05 '11 at 14:30
  • Even `Buffer[BytesRead+1] := #0;` might raise an exception, and without a `try..finally` approach, the handle will remain opened but useless for the rest of the life of your application... – Andreas Rejbrand Jul 05 '11 at 14:33
  • @Cosmin: no, the question is can closing the handle fail. – Witness Protection ID 44583292 Jul 05 '11 at 14:54
  • @Andreas: i don't think HTTPSendRequest or InternetReadFile or even Buffer[BytesRead+1] := #0 will ever generate an exception in the routine as coded. – Witness Protection ID 44583292 Jul 05 '11 at 14:57
  • @mike: You never know. Its like wearing seatbelts. Most of the time, it is just plain unnecessary, but it is worth it, because there is always a risk of an accident. The whole point of exceptions are that they should be 'exceptional'. Minor errors, like incorrect parameters, etc., are delt with by return values. Major accidents, such as access violations, are dealt with using exceptions. Sure, you don't need to write quality-proof code, but I prefer you do. In addition, the `try..finally` also lets you *exit* the procedure by raising an exception without risking a handle to remain opened. U... – Andreas Rejbrand Jul 05 '11 at 15:03
  • ...even accepted David's answer which *does* use `try..finally`! – Andreas Rejbrand Jul 05 '11 at 15:03
0

Looking at SOAPHTTPTrans, there are many calls to InternetCloseHandle. Although many appear within Except blocks, none are PROTECTED by one. So it appears that Codegear (mine is D2006) assumes that it won't fail.

Chris Thornton
  • 15,620
  • 5
  • 37
  • 62