src: more debug statements #708
Conversation
Add debug statement (enabled only in debug mode) at vital session life cycle control flow points Refs: https://github.com/expressjs/session/issues/633#issuecomment-471110093
index.js
Outdated
| @@ -393,6 +395,7 @@ function session(options) { | |||
| debug('saving %s', this.id); | |||
| savedHash = hash(this); | |||
| _save.apply(this, arguments); | |||
| debug('session saved'); | |||
There was a problem hiding this comment.
This prints before the session is saved, as the saving operation is async.
There was a problem hiding this comment.
This is based on I am assuming that this line is desired to be similar to the destroy logs, with the behavior of "destroying.." being logged before the store call and then "... destroyed" called when the store returns back to this module without an error.
There was a problem hiding this comment.
thanks @dougwilson . yes, the desire is to follow destroy. the intent is simple - many issues in this repo are about session not being saved or inability to know about it.
thinking about it more, I can achieve this at the save site only via structural changes to the save function. that is, anchoring the save call with a custom callback, check for errors, or else print the debug message and then invoke the original callback, if any.
Before implementing it and submitting for your review, I want to pro-actively check with you on it - will you be open for such a change?
There was a problem hiding this comment.
Hi @gireeshpunathil yes, that sounds good to me, to just mimic the destroy way in the save area.
There was a problem hiding this comment.
@dougwilson - done, pls have a look. Here is a sample output from a simple program with debug on, for your reference:
express-session fetching qs3pPenGDQy0hOXWCw_G8lD_ZLJw5AA1 +0ms
express-session no session found +5ms
express-session session generate +1ms
express-session saving 7G28RiMV20yaNOzU3UeC5_qJMKek7X6f +4ms
express-session set-cookie connect.sid=s%XXX; Path=/; Expires=YYY; HttpOnly +4ms
express-session session saved +3ms
There was a problem hiding this comment.
All the reviews here are just about having the new/changed code match StandardJS format as part of the continued move to StandardJS format for all our projects. I would normally push the format changes myself, but I know from past issues you wanted to be aware, so tried to use the suggested changes feature instead :)
My only other comment looking through all the changes is I see you added the word "session" the the destroying/destroyed debug statements, but did not do so for the touching/touched ones. Any reason not to add to those so they are all consistent?
|
For what reason? Do you think this is not a low priority against the other aspects the team is attempting to move forward like Express 5.0? I'm trying to apply tags around priorities to help people focus the appropriate attention to get larger efforts like 5.0 moving forward. IMO adding some extra debug wording seems low priority to me, compared to things like new features, bug fixes, etc. The tag itself doesn't mean that it gets no attention, just that it is comparatively low priority. I'm happy to remove it, but just unsure on the reasoning for removing it, as you didn't provide any. |
|
I am not arguing with you; but I cannot find a reason why this is a low priority: the reasoning for this change is provided in the first comment - to improve self diagnosis of a class of issues that report malfunction of session life-cycle events, more specifically users suspecting missing session persistence. Plus, there are 27 other PRs in this repo, and none of those are low pririty items! But I am defenitely not arguing with you nor questing your judgement. Also, if this is to compete with express 5.0 for project's attention and focus, I will prefer the later, as that has more user demand than the former. |
|
I already provided the reason why it is low priority above :) The tag has nothing to do with the timeline for landing the change; it is simply to provide a category of the change itself. The argument that the others are not tagged right is not an argument this is not a low priority change; getting things tagged correctly is actually your primary function as a triager :) So if other PRs are not tagged correctly, it sounds like that is an issue that needs t be addressed with at least the triagers; I was simply tagging this PR since I happened to just interact with it. If it is more important for me to go through all the issues and PRs in this repository and get them all tagged appropriately vs working on other items, I can certainly do that, but the idea was that we're bringing on the large group of triagers to perform these kinds of tasks such that folks like myself can actually focus on the larger-at-hand items. |
I have given response to that - it is not some random debug statements; it is derived from an insight from this repo's issue backlog.
There are PRs which are 6 years old with no activities for as long; and acting on those in a meaningful and timely manner is actually your primary function as a maintainer :) But then let us agree that we are not here to remind each other on their responsibilities.
It looks incorrect approach to me. If you just happened to interact with a work item cannot be used to derive a priority decision? Priorities are always relative; without looking at other PRs, a 'low priority' item cannot stand in itself. Low against what? |
I get it, but we're also not even actively telling folks to use what is there when triaging issues (see recent issues like #763 ) I think that this conversation is getting a bit out of hand and off topic, and frankly into a bit of the offensive realm. If you could, I would suggest if you could please step away for a bit. I'm going to lock this PR for now. |
|
@gireeshpunathil I think at this point going forward, I just can no longer act on any issues and PR you raise. You will need to get something else involved to move forward with your issues and PRs, as I do not feel comfortable in interactions between us. |
dougwilson
force-pushed
the
master
branch
2 times, most recently
from
9d2e29b to
408229e
Compare
Add debug statement (enabled only in debug mode) at vital session life cycle control flow points
Refs: https://github.com/expressjs/session/issues/633#issuecomment-471110093
Generally improves ability to diagnose issues.
(from a high level view on the outstanding issues in this repo, it is evident that the most prominent type is diagnosing session persistence across client requests, so this is a small step towards helping there)