Enqueue requests made from QuickIcon plugins #38019
Conversation
joomla-cms-bot
added
NPM Resource Changed
|
does the same thing need to be done/checked on the system dashboard? |
|
Yes. Now that you mention it, we do the same thing there too. Let me take a look and amend the PR. |
|
Thanks. FYI the discover check on that page does not work correctly anyway as it only displays the existing value. If the component hasnt been opened yet then the check reports nothing to discover |
AJAX badges appear in custom dashboards rendered by com_cpanel, e.g. the System Dashboard.
|
@brianteeman I fixed the AJAX badges' requests. About the Discover check, can you please open an issue and at-mention me? I will take a look but it's a completely different thing than what I am solving in this PR. You know what they say about scope creep 🤣 |
|
I only mentioned it so that you didnt waste time wondering why it didnt work ;) |
|
The discover check issue comes from our new bonus program "pay for one bug and get the other one for free" 😄 |
|
@brianteeman I was vaguely aware something is wrong but it never bothered me enough to deal with it. Well, I seem to have a couple of days with nothing overly pressing to do so I might just as well use them to fix some Joomla issues. Report it, tag me and I'll bring you its head on a stick 😉 @richard67 I don't think I signed up for this bonus programme 🤨 😆 |
|
As this is a bug fix shouldnt this pr be against 4.1? |
|
It does add a new feature in the |
|
stop talking about vacations - you make me sad |
|
I have tested this item ✅ successfully on 7bb0534 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38019. |
|
I think it is better to make a new method, kind of |
|
@Fedik What you propose is a worse monster. We'd have to duplicate Joomla.Request's code because that function does not return the options or the data, it only returns the XHR. Moreover, XHR does not allow you to extend the Further to that, what you propose would not work for the simple reason that you can and should be able to enqueue a request while another one is in progress. What you propose would only work if you enqueue requests before triggering any of them. In fact, the way you describe it you could only enqueue requests coming from one source. Each plugin enqueueing its own requests would result in all queues being processed in parallel... which is the exact problem we are trying to solve here! I took all of these issues into account before deciding to go for a minimal modification of Joomla.Request. Sure, we could pull the In any case, I just added 10 lines of code (count them!) in Joomla.Request. It's a far cry from making Joomla.Request a monster. Doing duplicated code would add well over 100 lines to core.es6.js, most of which duplicated, something which I would totally agree is a bloody monster! I chose DRY over the typical Joomla PR approach of "bash something together and pretend there's nothing wrong with it". |
const queue = [];
const checkQueue = () => {
const options = queue.shift();
if (!options) return;
options.perform = true;
Joomla.request(options);
};
Joomla.enqueueRequest = function (options) {
const onSuccess = options.onSuccess;
const onError = options.onError;
options.perform = false;
options.onSuccess = (response, xhr) => {
onSuccess && onSuccess(response, xhr)
checkQueue()
};
options.onSuccess = (xhr) => {
onError && onError(xhr)
checkQueue()
};
queue.push(options);
checkQueue();
}How much I duplicate of |
|
@Fedik You are a release lead and have commit rights. You can't possibly be writing this kind of bad code, can you? Sheesh! Okay, let me tell you everything that's wrong with your code just by reading it twice over. If the onSuccess or onError handler throws then you never call checkQueue(). That's a brand new bug. In my code the next request is launched before we call the onError / onSuccess handlers exactly so we avoid this problem. Further to that, what happens if I enqueue a request while another is running? Both requests run in parallel because checkQueue only checks if there are requests in the queue, not if there are other requests already executing. Therefore you are NOT fixing the problem we started to fix with this change. That's why I have a flag for request execution. You have added more code than I did. By the time you fix the issues I told you about you are at twice the number of lines of code. Your solution is not economical. You require the developer to use a different method (Joomla.enqueueRequest) which also does not return the XMLHttpRequest object. When used outside of core code (which admittedly does not use the XHR object as far as I remember) it's useful to have a reference to the XHR object. And now let's close in for the kill. You have created a magic, invisible tie-in between Joomla.enqueueRequest and Joomla.request. If at some point Joomla.request is rewritten to use fetch() instead of XMLHttpRequest your Joomla.enqueueRequest fails and can't be easily refactored. So, let's recap all the problems your code has and mine does not:
|
@nikosdion He's just a maintainer like I am, not a release lead. |
|
@richard67 Dang, right. Tobias is the release lead for 3.10, not Fedir. My apologies @Fedik I misremembered. It's hard to remember the release leads with 3.10, 4.1, 4.2 and 5.0 being in active development. I am bad at remembering names as it is... |
|
I'm here with @nikosdion. Better to introduce a new flag as 10 new lines is not that worse for queue support. I was just wondering if this can also be solved with promises in less than 10 lines 😄 |
That details, just switch positions: call checkQueue() first, and the calback is second, easy.
Who need queued request, they do queued reques, other request going as normal.
At some point Joomla.request should return Promise, or be removed at all (in favor to use fetch()). Here Any Queue or Ordering manipulation should be handled outside of Request. I do not mind if it will be merged as it is, I have wrote my opinion 😉 |
THIS! Joomla 4 should never had rolled a wrapper on a legacy JS method. Fetch is the way to go. Also doing sequential fetches is as easy as reducing the array of fetches... |
|
@laoneo I believe promises can be more economical but I don't have enough hands-on experience to confidently say I can do it right. @Fedik you missed something important when you said this:
Please re-read the issue referenced in this PR. Okay? You did? Good. The problem is that we have a bunch of QuickIcon plugins which do not know about each other, all sending a request one after the other. Their JavaScript files which do not and must not know about each other are executed faster than it takes for a request to finish executing. Therefore we end up having 4-5 requests being sent from the browser within half a second. This triggers the DoS protection of the server, some of these requests fail and users perceive Joomla as broken. We need a magic way which will "hold onto" these requests and only fire them after the previous one has finished executing. That's the purpose of the queueing. As implied from the above description, a new request may be added to the queue while a previous one is still executing. You must not execute it just yet! This is what my code does and yours does not. We need to execute the new request after the previous one finishes, be it successful or failed. If we need to have the caller determine if the request needs to be executed our plugins are no longer DRY. They need to know about each and every other element on the page which might be executing requests, somehow access these xhr objects, inspect their status and decide if they are going to execute their own request. Not only this is convoluted and bad architecture, it also makes it impossible to work with third party plugins which by definition cannot know about each other and the core code cannot know about them! Again, this is only possible with a FIFO queue which waits for request execution. @dgrammatiko Yup, this kind of refactoring is on my radar. I need some more experience with fetch though before doing that 😉 I also don't think that reducing the array would work because we need to wait for each request to finish before sending the next one. Unless you mean using await in the reducer? This is the part I don't have a lot of hand-on experience with and why I didn't dare convert this to fetch() just yet. I fully agree that core JS should be much lighter. Ideally all that code would be modules and the 3PD and core code would add them as dependencies in the |
|
@nikosdion there's already a piece of code in Joomla showcasing this reducing promises: But fwiw this kind of functionality should not be part of core.js, it's extremely rare the need of sequential fetches and when it's needed the solution is couple of lines (the Array.reduce() part) |
Hm? My code do queued request in FIFO order. |
|
@Fedik I was trying hard to give you an out without embarrassing you too much (not more than posting obviously broken code with a snide remark already did) but you won't let it go. OK, if you insist... As stated in #38001 the root cause of the problem is the execution of multiple requests overlaps. This results in too many requests being executed in a short period of time which is perceived by the web server as a Denial of Service attack. As a result the server blocks requests from that IP address for a few seconds to a few hours. Therefore the requests fail to execute and the user perceives Joomla as broken. The solution to that is to somehow make these requests NOT overlap by executing them consecutively AND NON_OVERLAPPING, ergo each request CANNOT start before its previous has finished executing. You are not doing that. You are immediately processing the queue, plucking the next item from the top of the queue as soon as you add it and immediately executing it. There is no check that the request you've already started has finished executing, therefore your requests overlap just like if you were using Joomla.Request directly. If you try your code and look at your browser's dev tools Network pane you will see the same graph as the "Actual result BEFORE applying this Pull Request" in this PR. Let me give you an example using these timings and what would happen with YOUR code: At 1.4s after the page loads, a piece of JS code adds options for a new request (A) which will take 0.135 seconds. You are immediately calling checkQueue(). This method plucks the topmost request of the queue and executes it. The request A is currently running and the queue is empty. At 1.45s after the page loads a piece of JS code adds options for a new request (B) which will take 2.9 seconds. You are immediately calling checkQueue(). This method plucks the topmost request of the queue and executes it. The requests A and B are currently running and your queue is empty. At 1.5s after the page loads a piece of JS code adds options for a new request (C) which will take 0.212 seconds. You are immediately calling checkQueue(). This method plucks the topmost request of the queue and executes it. The requests A, B and C are currently running and your queue is empty. At 1.55s after the page loads a piece of JS code adds options for a new request (D) which will take 0.306 seconds. You are immediately calling checkQueue(). This method plucks the topmost request of the queue and executes it. The requests A (about to finish), B, C and D are currently running and your queue is empty. This is exactly the same effect as calling Joomla.Request directly! You did not implement a queue, you did not solve the problem. Congratulations for drilling a big hole in the water. @dgrammatiko Ah, I see what you mean. |
And this makes use of |
|
@Fedik the fact that the array is not mutatable should not be a problem if you think how to orchestrate the whole procedure. Eg: const queueOfFetches = [];
function enqueueFetches (fetch) {
queueOfFetches.push(fetch);
// Since the backend knows how many buttons were added we pass the count($buttons) to the JS
// And we fire the orchestration when all the scripts were pushed in the queue
// something like:
if (Joomla.getScriptOptions('fetch.buttons') === queueOfFetches) {
queueOfFetches.reduce(....
}
}Each button SHOULD call the function above instead of resoling the fetch on page load or any other event |
|
@dgrammatiko I would very much prefer it if we did not have the backend know everything about how the frontend works and have it orchestrate things. I was thinking something along the lines of https://gist.github.com/nikosdion/908156641a83ab4b849ecaae9f42f48a (obviously needs some rejection handling, this is the minimum viable proof of concept I knocked out in like 5') |
That not a Jedi way hehe |
Here's another way: document.addEventListener('DOMContentLoaded', () => {
queueOfFetches.reduce(....
});The list of ways to orchestrate this are many more than these two. My point is that the functionality is exclusive to this module (joomla module not js module) and the actual implementation with modern js is just a couple of lines. |
|
@nikosdion I was very close to what you made https://gist.github.com/nikosdion/908156641a83ab4b849ecaae9f42f48a I have missed to override variable with My result kind of this: function fakeFetch (n) {
return new Promise((resolve, reject) => {
//resolve('Works ' + n)
setTimeout(() => {
resolve('Works ' + n)
}, 1000 * n);
});
}
const chain = {};
Joomla.chainPromise = function (group, promise) {
if (!chain[group]) chain[group] = Promise.resolve();
let prev = chain[group];
prev = prev.then(() => {
return Promise.resolve(promise)
});
chain[group] = prev;
return prev;
}
Joomla.chainPromise('fetch', fakeFetch(1)).then((msg) => {
console.log(msg);
})
Joomla.chainPromise('fetch', fakeFetch(2)).then((msg) => {
console.log(msg);
})
Joomla.chainPromise('fetch', fakeFetch(3)).then((msg) => {
console.log(msg);
}) |
|
@Fedik Yup, I have a slightly simpler way, too. Give me a sec, I want to check something. |
|
The bulk of it is this. Joomla.Request would use fetch and return its promise. Then all we need is: let lastRequestPromise = Promise.resolve();
Joomla.enqeueRequest = (options) => {
lastRequestPromise = lastRequestPromise.then(() => Joomla.Request(options));
}I would also say that in the context of what Joomla.Request is meant to do it doesn't make sense to have multiple queues as they can be abused to the point that we come back to the problem of too many simultaneous requests. If we do want to do that we could possibly do let requestQueue = {};
Joomla.enqeueRequest = (queueName, options) => {
requestQueue[queueName] = (requestQueue[queueName] ?? Promise.resolve())
.then(() => Joomla.Request(options));
} |
|
yea
True, it does not need to be grouped when it only for And I thought about changing Joomla.Request. Btw, |
|
@Fedik Correct, there is no way to track upload progress with fetch, only download progress. So, at the very least, we will need to keep Joomla.Request as-is because installing extensions requires upload tracking. I will do another PR to introduce Let's leave this PR here the way it is. There are legitimate uses cases for people to want queued execution with Joomla.Request. At the very least I have one in Akeeba Backup 🤣 If this PR works for you please do submit a successful test. |
Please DON'T! There's literally no benefit to namespace or wrap a native method. Also the Docs both in MDN and web.dev are way better than any non existent docs of the Joomla.request (or any other method, tbh). About the The point is that My 2c |
This is wrong, I'm afraid. We need to magically add the Moreover, if we want to make it possible to queue requests we CAN NOT use I think can just add Joomla.enqueueRequest({
url: 'index.php?option=com_example&view=ajax&format=json'
})
.then(response => response.json())
.then(data => {
// Do something
});The That's my overall idea, probably I need to change a few things. It's still very much in the concept stage. |
|
I have tested this item ✅ successfully on 7bb0534 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38019. |
It's a line in the options, you seriously DO NOT need to wrap every native method but whatever...
Wrong, all you need is an API to push the fetch in an array and that array will be then executed sequentially |
|
@dgrammatiko Since you know better why don't you do the PR? Problem solved. |
There's no v5 Repo... |
|
@dgrammatiko As we discussed above, we'd need to keep So whatever you add will be a new method, e.g. |
|
And to clarify: I am not being sarcastic, I HONESTLY want to see how it's done! |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
I will say this a final time. Our requirements are really simple:
Dimitris' method cannot work without violating one of these conditions. If you want to fulfil the first condition you need to do something like https://decembersoft.com/posts/promises-in-serial-with-array-reduce/ which only works if you know about all of the requests in advance. This violates the second condition. If you fulfil the second condition you are pushing the promises returned by fetch() to an array. However, this does not prevent the request from starting, violating the first condition. Here's a simple POC and note that I'm not even trying to do anything with the array: const fetchQueue = [];
for (let i = 0; i < 5; i++)
{
fetchQueue.push(fetch('https://www.exaxmple.com'));
}Result: All the requests started executing immediately. We never even had the change to reduce the array. I still don't see how you can possibly run fetch without running the request immediately when, um, this is exactly what it's supposed to do per https://developer.mozilla.org/en-US/docs/Web/API/fetch and I quote:
Even the WhatWG specification says it will start the request immediately: https://fetch.spec.whatwg.org/#fetch-method As for using fetch... If you don't want to enqueue, use fetch(). If you want to enqueue and/or want upload feedback use Joomla.Request. No further changes necessary in the core code. There is no point trying to write something convoluted which will reintroduce the problem we just fixed in this here PR. Just merge this PR and that's all there is to it. Over and out and have a good night, y'all! |
|
Thanks everybody |
|
This is still an issue in Joomla! 4.2.6 |
Pull Request for Issue #38001.
Summary of Changes
Requests made by QuickIcon plugins are now enqueued and executed serially (each requests starts after the previous one has finished), thereby avoiding triggering the server's Denial of Service protection.
Testing Instructions
Log into your Joomla backend.
Actual result BEFORE applying this Pull Request
All QuickIcon requests execute at the same time.
In the Network tab of your browser's developer tools filter by XHR / Fetch and you will see something like this:
Note how the start of each request (green bars) overlap. This causes many live servers to trigger their Denial of Service protection because they see "a lot" of requests coming from the same IP at very close temporal proximity.
Expected result AFTER applying this Pull Request
The QuickIcon requests execute serially, one after the other.
In the Network tab of your browser's developer tools filter by XHR / Fetch and you will see something like this:
Note how the start of each request (green bars) is at the tail end of the previous one. Since there is only one request at any given time being handled by the server it's far less likely to trigger the Denial of Service protection.
Documentation Changes Required
None.