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

Add session expiry #6

Merged
merged 7 commits into from
Apr 30, 2023

Conversation

kokonutwh1skey
Copy link
Contributor

  1. Add session expiry
  2. Reorganize files
  3. Fix typos in README.md

@AlexeyAkhunov
Copy link
Contributor

thank you! I had a quick look and there are couple of issues you may want to look into:

  1. There are some stray dependencies in go.sum which you may want to clean up using go mod tidy
  2. The expiration is currently only checked when the the session is being accessed, and this is not a good time to expire the session, because it means someone needs it. This also does not clean up the sessions that have not been accessed for a long time.

I suggest that you try to organise sessions in a priority queue, prioritised by the time since the last access (you can use container/heap for that), and introduce a configurable limit of how many active sessions there may be at any given time. That way, oldest sessions will be removed when room for a new session is required.

@kokonutwh1skey
Copy link
Contributor Author

thank you! I had a quick look and there are couple of issues you may want to look into:

  1. There are some stray dependencies in go.sum which you may want to clean up using go mod tidy
  2. The expiration is currently only checked when the the session is being accessed, and this is not a good time to expire the session, because it means someone needs it. This also does not clean up the sessions that have not been accessed for a long time.

I suggest that you try to organise sessions in a priority queue, prioritised by the time since the last access (you can use container/heap for that), and introduce a configurable limit of how many active sessions there may be at any given time. That way, oldest sessions will be removed when room for a new session is required.

I think LRU is a good candidate to implement it.

@AlexeyAkhunov
Copy link
Contributor

I think LRU is a good candidate to implement it.

Yes, definitely LRU cache from a library can also be used

@kokonutwh1skey
Copy link
Contributor Author

Updated PR with LRU

@AlexeyAkhunov
Copy link
Contributor

Should UiSessions also be turned into LRU cache? Is the lock for the nodeSessionMap still required? Default value of max.sessions of 50 is way too low, I would say 5000 :)

@kokonutwh1skey
Copy link
Contributor Author

Should UiSessions also be turned into LRU cache? Is the lock for the nodeSessionMap still required? Default value of max.sessions of 50 is way too low, I would say 5000 :)

Yeah, I missed the UiSessions.

Updated pr:

  1. LRU for ui sessions
  2. Remove locks for both uiSessions and nodeSessions maps
  3. Changed the default value to 5000

@AlexeyAkhunov
Copy link
Contributor

Thank you, I will take a look shortly. Now that we have linter, could you please also fix lint errors?

@kokonutwh1skey
Copy link
Contributor Author

Fixed lint errors

@AlexeyAkhunov
Copy link
Contributor

Thanks, can you also tidy up go.sum with go mod tidy?

@kokonutwh1skey
Copy link
Contributor Author

Thanks, can you also tidy up go.sum with go mod tidy?

I ran go mod tidy but nothing changed.

@AlexeyAkhunov AlexeyAkhunov merged commit b9de59b into erigontech:main Apr 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants