In reading through drupal.js, I noticed two minor problems with HTTPGet().

1) There is a race condition created by calling send() before setting the callback function. Admittedly, this race shouldn't be lost, but it's not good practice to bet on winning races in code.

2) Line 55 is redundant. Since the value of bAsync is based on the non/existence of callbackFunction, there is no need to check both variables later.

The attached file removes both the race condition and the redundancy.

CommentFileSizeAuthor
#4 httpget_httppost.patch1.37 KBAnonymous (not verified)
HTTPGet.js1.09 KBSid_M

Comments

Sid_M’s picture

Sid_M commented

The same issues are found in the HTTPPost function.

Egon Bianchet’s picture

Egon Bianchet commented
Version: 4.7.0-beta4 » 4.7.0

Please provide a patch (patch howto).

Egon Bianchet’s picture

Egon Bianchet commented
Category: task » bug

I think it's a bug report

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new httpget_httppost.patch1.37 KB

I don't agree with moving the send() function.. it's fine where it is. However, I agree with the redundancy on the multiple if statements based on bAsync.

Here is a patch that removes the reduancy but keeps the functions how they were.

Egon Bianchet’s picture

Egon Bianchet commented
Status: Active » Needs review
dries’s picture

dries commented

Code looks good but how can one reproduce the problem, and test that this works?

kkaefer’s picture

kkaefer commented
Status: Needs review » Reviewed & tested by the community

Patch looks good. +1 on this. It's just for "cosmetic" purposes and does not alter any functionality whatsoever.

killes@www.drop.org’s picture

killes@www.drop.org commented
Version: 4.7.0 » x.y.z

committed to 4.7

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)
ddoongoor’s picture

ddoongoor commented

I think this is still an issue. I've just downloaded 4.7.3 and misc/drupal.js still contains HTTPGet() (and HTTPPost()) with the possible race condition.

killes@www.drop.org’s picture

killes@www.drop.org commented

the fix will be part of a future 4.7.4 release

kkaefer’s picture

kkaefer commented

In addition, this issue does no longer exist in HEAD as we use jQuery for all AJAX functionalities.

ddoongoor’s picture

ddoongoor commented

Thanks for the update. In our project we've actually seen the race condition manifest itself. We've applied this revision to our version and it corrects the problem.