Thanks so much for the breakdown. I got the code from easycfm.com and didn't think to really analyze it. I'll make the changes you suggested and let you know if I run into anything. Thanks again!
- Sung
>Sung,
>
>> What happens is that at the end of the day (I run an intranet where
>there
>> are about 200 peak users), around midnight, it still shows 80 or so
>users
>> online! What do you think could be causing this?
>
>Does the code you run to "report" on the user sessions run the code you
>posted first? By this I mean does it run through and clear any old
>sessions out before reporting how many are still there?
>
>What the code you posted does (badly) is record a timestamp of when a
>new session is started, for each user session (although the CFID is by
>no means guaranteed to be unique), and then deleting any over 15
>minutes. So every session will remain there for 15 minutes (or until
>the first page call that runs the skimmer code after the 15 minute limit
>has expired) and then be deleted. If the user is still using the
>system, they will simply be added back in the next time they access a
>page, with a new 15 minute "count".
>
>Therefore, at the end of the day, if there are no page calls made
>between 5:30 and midnight, all sessions will still be held in memory,
>because the skimmer hasn't been run. I would say that's why you're
>still leeing lots of users on line at midnight.
>
>I'd like to ask what you're actually attempting to do with this code.
>If it's to see how many people are using the system within the last 15
>minutes and how long they've been using the application for, then this
>code certainly doesn't do that, or at most does a very bad job of it.
>What you'll need to do is add *two* timestamps to each user -- one for
>when they first accessed the system, and one when they last requested a
>page. Your "15 minute check" should then use the difference between
>these values to determine whether the user has been inactive for 15
>minutes.
>
>As for Ray's comment of "but that is some pretty awful code" I
>completely agree. I've attempted to go through it somewhat below
>code-block by code-block -- not changing any of the existing
>functionality, but just cleaning it up -- with explanations of what I've
>done and why.
>
>> <cflock timeout="15" scope="APPLICATION" type="EXCLUSIVE">
>> <cfif NOT isDefined("Application.UsersInfo")>
>> <cfset Application.UsersInfo = StructNew()>
>> </cfif>
>> </cflock>
>
>Since CFMX (I'm assuming you're using MX?), you don't have to worry
>about locking shared scopes for reading only, so there's no need to lock
>everything as you have done. Put another <cfif> around so you only lock
>when absolutely necessary. Keeping the original <cfif> in there just
>makes sure that should two requests happen fairly closely, that only one
>initialises the new structure.
>
><cfif NOT isDefined("Application.UsersInfo")>
> <cflock timeout="15" scope="APPLICATION" type="EXCLUSIVE">
> <cfif NOT isDefined("Application.UsersInfo")>
> <cfset Application.UsersInfo = StructNew()>
> </cfif>
> </cflock>
></cfif>
>
>> <cflock name="#CreateUUID()#" timeout="15" type="EXCLUSIVE">
>> <cfset user_cfid = Evaluate(CFID)>
>> <cfset user_time = Now()>
>> </cflock>
>
>This is a totally pointless lock. You're not writing to (or even
>reading from) any shared scope. Additionally, you're using a randomly
>generated lock ID so even if you were writing to a shared scope there's
>nothing to stop two requests happening simultaneously, because they are
>guaranteed to have different lock IDs!
>
>Secondly, I'm not sure why you're using an evaluate() call where you
>are, it doesn't do anything except add another function call. The
>user's CFID is available anywhere and won't change so there's no need to
>reference that as anything different. The user_time variable is only
>actually used once, so why not just replace it with the now() call? In
>effect, you don't need this section at all.
>
>> <cflock scope="APPLICATION" type="EXCLUSIVE" timeout="15">
>> <cfif NOT StructKeyExists(Application.UsersInfo, user_cfid)>
>> <cfset temp = StructInsert(Application.UsersInfo, user_cfid,
>user_time)>
>> </cfif>
>> </cflock>
>
>Again, as in the first section, there's no need to lock for a read, so
>add another <cfif> statement around the outside of this section.
>Replace the user_cfid with CFID and the user_time with now() as
>described above. Finally, you don't have to assign the result of the
>structinsert() call -- it just means another variable clogging up memory
>during the page request:
>
><cfif NOT StructKeyExists(Application.UsersInfo, CFID)>
> <cflock scope="APPLICATION" type="EXCLUSIVE" timeout="15">
> <cfif NOT StructKeyExists(Application.UsersInfo, CFID)>
> <cfset StructInsert(Application.UsersInfo, CFID, now())>
> </cfif>
> </cflock>
></cfif>
>
>> <cflock scope="APPLICATION" type="EXCLUSIVE" timeout="15">
>> <cfloop collection="#Application.UsersInfo#" item="itmUser">
>> <cfif
>> Evaluate(DateDiff("n", StructFind(Application.UsersInfo, itmUser),
>Now())) GT 15
>> >
>> <cfset StructDelete(Application.UsersInfo, itmUser)>
>> </cfif>
>> </cfloop>
>> </cflock>
>
>Now this is the sort of block where everyone's opinions will diverge
>somewhat. Some will say lock outside the loop, other will say only lock
>when deleting. Only locking when deleting will make the code "cleaner",
>but *may* leave you open to race conditions where two instances of the
>script are running at the same time and try to read and delete from the
>same item in the structure at the same time.
>
>By locking the whole loop, if you have a large number of users and page
>requests, you might be asking every script to wait while 200 "users" are
>looped through. Obviously it's a fairly trivial number in this case,
>but the theory is that you wouldn't do that on something with possibly
>1000s of sessions, so why in this case?
>
>Personally, I'd change things a bit and use a named lock inside the
>loop, with a <cfif> call inside that to make sure that we're not trying
>to read or delete something that isn't there any more. Note I've also
>removed the unrequired evaluate() call:
>
><cfloop collection="#Application.UsersInfo#" item="itmUser">
> <cflock name="session-lock-#itmUser#" timeout="15">
> <cfif structkeyexists(Application.UsersInfo, itmUser)>
> <cfif DateDiff("n", Application.UsersInfo[itmUser)>], Now())
>GT 15>
> <cfset StructDelete(Application.UsersInfo, itmUser)>)>
> </cfif>
> </cfif>
> </cflock>
></cfloop>
>
>I hope that's some help! Now I'll get back to doing something I'm being
>paid to do... ;)
>
>Tim.
>
>--
>-------------------------------------------------------
>Badpen Tech - CF and web-tech: http://tech.badpen.com/
>-------------------------------------------------------
>RAWNET LTD - Internet, New Media and ebusiness Gurus.
>WE'VE MOVED - for our new address, please visit our
>website at http://www.rawnet.com/ or call us any time
>on 0800 294 24 24.
>-------------------------------------------------------
>This message may contain information which is legally
>privileged and/or confidential. If you are not the
>intended recipient, you are hereby notified that any
>unauthorised disclosure, copying, distribution or use
>of this information is strictly prohibited. Such
>notification notwithstanding, any comments, opinions,
>information or conclusions expressed in this message
>are those of the originator, not of rawnet limited,
>unless otherwise explicitly and independently indicated
>by an authorised representative of rawnet limited.
>-------------------------------------------------------
[Todays Threads] [This Message] [Subscription] [Fast Unsubscribe] [User Settings] [Donations and Support]
