Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ended the request (and close the session) before core_app_run_after event #1592

Merged
merged 4 commits into from
Apr 5, 2023

Conversation

colinmollenhour
Copy link
Member

@colinmollenhour colinmollenhour commented May 3, 2021

Description (*)

This PR does two things:

  1. Explicitly ends the request and closes the session after the app has completed rendering.
  2. Adds core_app_run_after event to allow events to occur after response is sent.

With this PR you can add an event observer for core_app_run_after which can do something which will not add any time to the response even if it is long-running (e.g. sleep(3);).

Related Pull Requests

Closes #113

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

…mpleted rendering. Add core_app_run_after to allow events to occur after response is sent. Refs OpenMage#113
@github-actions github-actions bot added Component: Core Relates to Mage_Core documentation labels May 3, 2021
@fballiano
Copy link
Contributor

the core_app_run_after event won't be able to access any session based data right?

@colinmollenhour
Copy link
Member Author

the core_app_run_after event won't be able to access any session based data right?

I've not tried this.. I think the $_SESSION variable is still present with the same values but I'm guessing that modifying it would simply have no effect.

@luigifab
Copy link
Contributor

luigifab commented May 4, 2021

Before:
image
After:
image

After, __destruct are called after response flushes.

@joshua-bn
Copy link
Contributor

In general, this makes me uneasy. I would offload anything that I wanted to run after to a queue.

@Flyingmana
Copy link
Contributor

the core_app_run_after event won't be able to access any session based data right?

I've not tried this.. I think the $_SESSION variable is still present with the same values but I'm guessing that modifying it would simply have no effect.

its described in this comment here: https://www.php.net/manual/en/function.session-write-close.php#112681

An easy gotcha here - the $_SESSIONS superglobal does not vanish because you call session_write_close. If subsequent to the write_close call if you manipulate the superglobal the changes will not be saved when the script exists. Likewise a call to session_regenrate_id will fail.

So, are we sure there is noone who still access the session after this here was called (in index.php)?

also its probably better to check for the session status instead of the variable https://www.php.net/manual/en/function.session-status.php

@luigifab
Copy link
Contributor

luigifab commented May 11, 2021

I tested this PR to sync data after each orders.
But to know if the current order is new, I have added:

diff --git a/app/code/core/Mage/Sales/Model/Service/Quote.php b/app/code/core/Mage/Sales/Model/Service/Quote.php
index cf24261eaa..112b74a1f4 100644
--- a/app/code/core/Mage/Sales/Model/Service/Quote.php
+++ b/app/code/core/Mage/Sales/Model/Service/Quote.php
@@ -182,7 +182,8 @@ class Mage_Sales_Model_Service_Quote
 
         Mage::unregister('current_order');
         Mage::register('current_order', $order);
-        
+        $order->setData('new_order', 1);
+
         /**
          * We can use configuration data for declare new order status
          */

Not sure if there is another solution?


I also getting a new problem. We are generating our product images with multi threads. With this PR, it continue to work, but, we are waiting all threads to end in a __destruct. Because now __destruct is triggered after response is sent, customers can see blank images when images are not yet generated.

I can fix my problem with a new event before fastcgi_finish_request or by using controller_front_send_response_before event.

luigifab
luigifab previously approved these changes Dec 14, 2021
Copy link
Contributor

@luigifab luigifab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In production for > 4 months.
We must also update EVENTS.md.

@fballiano fballiano changed the base branch from 20.0 to main April 4, 2023 17:31
@fballiano fballiano dismissed luigifab’s stale review April 4, 2023 17:31

The base branch was changed.

luigifab
luigifab previously approved these changes Apr 5, 2023
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated EVENTS file

@fballiano fballiano changed the title End the request and close the session before core_app_run_after event Ended the request (and close the session) before core_app_run_after event Apr 5, 2023
@fballiano fballiano merged commit fa2b8c3 into OpenMage:main Apr 5, 2023
} else {
flush();
}
if (isset($_SESSION)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fballiano added a commit that referenced this pull request Apr 5, 2023
…vent (#1592)

* Explicitly end the request and close the session after the app has completed rendering. Add core_app_run_after to allow events to occur after response is sent. Refs #113

* Update EVENTS.md

---------

Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants