src: more debug statements #708

gireeshpunathil · 2019-11-04T09:18:49Z

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)

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

ghinks

Useful additions to the debug. Thank you Gireesh.

dougwilson · 2020-06-04T00:19:53Z

index.js

@@ -393,6 +395,7 @@ function session(options) {
        debug('saving %s', this.id);
        savedHash = hash(this);
        _save.apply(this, arguments);
+        debug('session saved');


This prints before the session is saved, as the saving operation is async.

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.

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?

Hi @gireeshpunathil yes, that sounds good to me, to just mimic the destroy way in the save area.

@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

dougwilson

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?

index.js

gireeshpunathil · 2020-07-10T14:38:11Z

I have addressed the review comments
I have added the word session in the debug statements in touch code
request to have a look
request to remove the low priority tag

dougwilson · 2020-07-10T14:43:50Z

request to remove the low priority tag

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.

index.js

gireeshpunathil · 2020-07-10T15:07:39Z

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.

dougwilson · 2020-07-10T15:12:53Z

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.

gireeshpunathil · 2020-07-10T15:31:51Z

I already provided the reason why it is low priority above :)

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.

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 :)

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.

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.

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?

dougwilson · 2020-07-10T15:43:55Z

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.

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.

dougwilson · 2020-07-10T15:49:51Z

@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.

src: more debug statements

07e732d

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

dougwilson reviewed Apr 17, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

dougwilson reviewed Apr 17, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

dougwilson reviewed Apr 17, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

dougwilson added the pr label Apr 17, 2020

fixup: address review comments

46edd76

ghinks approved these changes Jun 3, 2020

View reviewed changes

dougwilson suggested changes Jun 4, 2020

View reviewed changes

fixup: address review comments

b0a2f31

dougwilson suggested changes Jul 10, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

dougwilson added the low priority label Jul 10, 2020

fixup: address review comments

2d8776e

dougwilson suggested changes Jul 10, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

fixup: review comments

fd6a948

dougwilson reviewed Jul 10, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

fixup: address review comments

77b8e59

expressjs locked as too heated and limited conversation to collaborators Jul 10, 2020

dougwilson force-pushed the master branch 2 times, most recently from 9d2e29b to 408229e Compare January 28, 2024 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: more debug statements #708

src: more debug statements #708

gireeshpunathil commented Nov 4, 2019

ghinks left a comment

dougwilson Jun 4, 2020

dougwilson Jun 4, 2020

gireeshpunathil Jun 4, 2020

dougwilson Jun 11, 2020

gireeshpunathil Jun 11, 2020

dougwilson left a comment

gireeshpunathil commented Jul 10, 2020

dougwilson commented Jul 10, 2020

gireeshpunathil commented Jul 10, 2020

dougwilson commented Jul 10, 2020

gireeshpunathil commented Jul 10, 2020

dougwilson commented Jul 10, 2020

dougwilson commented Jul 10, 2020

src: more debug statements #708

Are you sure you want to change the base?

src: more debug statements #708

Conversation

gireeshpunathil commented Nov 4, 2019

ghinks left a comment

Choose a reason for hiding this comment

dougwilson Jun 4, 2020

Choose a reason for hiding this comment

dougwilson Jun 4, 2020

Choose a reason for hiding this comment

gireeshpunathil Jun 4, 2020

Choose a reason for hiding this comment

dougwilson Jun 11, 2020

Choose a reason for hiding this comment

gireeshpunathil Jun 11, 2020

Choose a reason for hiding this comment

dougwilson left a comment

Choose a reason for hiding this comment

gireeshpunathil commented Jul 10, 2020

dougwilson commented Jul 10, 2020

gireeshpunathil commented Jul 10, 2020

dougwilson commented Jul 10, 2020

gireeshpunathil commented Jul 10, 2020

dougwilson commented Jul 10, 2020

dougwilson commented Jul 10, 2020