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 SQLite3TokenManager and associated example #1692

Merged
merged 3 commits into from
Jun 14, 2021
Merged

Conversation

bboe
Copy link
Member

@bboe bboe commented Feb 24, 2021

No description provided.

@bboe
Copy link
Member Author

bboe commented Feb 24, 2021

There are legit issues only affecting Windows currently because it seems Windows in the GitHub actions environment does not permit the database file to be opened twice. Perhaps the Windows VM relies on NFS thus resulting in the problem as indicated in the following link:

https://www.sqlite.org/faq.html#q5

SQLite uses reader/writer locks to control access to the database. (Under Win95/98/ME which lacks support for reader/writer locks, a probabilistic simulation is used instead.) But use caution: this locking mechanism might not work correctly if the database file is kept on an NFS filesystem. This is because fcntl() file locking is broken on many NFS implementations. You should avoid putting SQLite database files on NFS if multiple processes might try to access the file at the same time. On Windows, Microsoft's documentation says that locking may not work under FAT filesystems if you are not running the Share.exe daemon. People who have a lot of experience with Windows tell me that file locking of network files is very buggy and is not dependable. If what they say is true, sharing an SQLite database between two or more Windows machines might cause unexpected problems.

Before getting the error with opening the file a second time, I tried using NamedTemporaryFile and Python simply had issues opening the first instance. So there is definitely something up with Windows, at least as it runs on GitHub actions.

@PythonCoderAS
Copy link
Contributor

PythonCoderAS commented Feb 24, 2021

I feel like any SQLite3 Token Manager should integrate with an existing file instead of making an entirely new database file. I feel like the class should therefore get the Sqlite3 connection object and the name of a table to use for storing the token.

Edit: It seems to make a table, so the second part is invalid. I still feel that accepting the Connection object directly is the best way to go, since you don't need to open the file multiple times.

@bboe
Copy link
Member Author

bboe commented Feb 24, 2021

@PythonCoderAS,

Thanks for the feedback, and welcome back!

SQLite3 Token Manager should integrate with an existing file instead of making an entirely new database file

I think the more common case is that users aren't already working with a SQLite database. If someone wants to integrate with an existing database, they are free to make their own subclass. The intent behind FileTokenManager and SQLiteTokenManager are not to be robust, feature-rich solutions, but merely simple examples which others can adapt as necessary.

I still feel that accepting the Connection object directly is the best way to go, since you don't need to open the file multiple times.

While that would resolve Windows the issue for the tests as written, that doesn't help if the same SQLite file is opened by separate programs, which is desirable. I think passing a Connection object also adds a bit of unnecessary complexity.

I think the solution to the Windows issue might be opening the connection only when necessary, and having a built-in retry if the file is locked, but that still needs to be investigated more.

@github-actions
Copy link

This PR is stale because it has been open for 20 days with no activity. Remove the Stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale Issue or pull request has been inactive for 20 days label May 20, 2021
@LilSpazJoekp LilSpazJoekp removed the Stale Issue or pull request has been inactive for 20 days label May 20, 2021
@github-actions
Copy link

This PR is stale because it has been open for 20 days with no activity. Remove the Stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale Issue or pull request has been inactive for 20 days label Jun 10, 2021
@LilSpazJoekp LilSpazJoekp removed the Stale Issue or pull request has been inactive for 20 days label Jun 10, 2021
@bboe
Copy link
Member Author

bboe commented Jun 11, 2021

I think I'll just exclude Windows from this example and the test. That should be better than simply dropping it altogether.

@bboe bboe force-pushed the sqlite_token_manager branch 3 times, most recently from 65199c5 to bb0e000 Compare June 11, 2021 03:37
@bboe bboe requested a review from LilSpazJoekp June 11, 2021 03:43
Copy link
Member

@LilSpazJoekp LilSpazJoekp left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few minor things.

praw/util/token_manager.py Outdated Show resolved Hide resolved
praw/util/token_manager.py Outdated Show resolved Hide resolved
praw/util/token_manager.py Outdated Show resolved Hide resolved
praw/util/token_manager.py Outdated Show resolved Hide resolved
praw/util/token_manager.py Outdated Show resolved Hide resolved
praw/util/token_manager.py Outdated Show resolved Hide resolved
docs/examples/use_sqlite_token_manager.py Outdated Show resolved Hide resolved
docs/examples/use_sqlite_token_manager.py Outdated Show resolved Hide resolved
docs/examples/use_sqlite_token_manager.py Outdated Show resolved Hide resolved
@bboe
Copy link
Member Author

bboe commented Jun 13, 2021

Thanks for the review and catching all those issues. I'd incorporated all your suggested changes.

Copy link
Member

@LilSpazJoekp LilSpazJoekp left a comment

Choose a reason for hiding this comment

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

Two tiny things to make the checks happy. Also, you'll need to resolve conflicts.

praw/util/token_manager.py Outdated Show resolved Hide resolved
praw/util/token_manager.py Outdated Show resolved Hide resolved
@bboe bboe force-pushed the sqlite_token_manager branch 3 times, most recently from bb0e002 to bb0e008 Compare June 14, 2021 02:29
@bboe bboe merged commit 29e7f00 into master Jun 14, 2021
@bboe bboe deleted the sqlite_token_manager branch June 14, 2021 02:53
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.

None yet

3 participants