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.

Sign up for GitHub

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

Jump to bottom

Evaluate whether userlist-thread lock is working #32

Closed
Krinkle opened this issue Oct 15, 2017 · 0 comments
Closed

Evaluate whether userlist-thread lock is working #32

Krinkle opened this issue Oct 15, 2017 · 0 comments
Assignees
@Krinkle
Labels
meta
Milestone
1.22

Comments

@Krinkle
Copy link
Member

Krinkle commented Oct 15, 2017 •
edited
Loading

Gendarme provides various warnings for the following type of issue:

190. WriteStaticFieldFromInstanceMethodRule
* Target:   System.Void CVNBot.ListManager::AddGroupToList()
* Details:  The static field 'currentGetThreadWiki', of type 'System.String'. is being set in an instance method.

..

192. WriteStaticFieldFromInstanceMethodRule
* Target:   System.String CVNBot.ListManager::ConfigGetAdmins(System.String)
* Details:  The static field 'currentGetThreadWiki', of type 'System.String'. is being set in an instance method.

199. WriteStaticFieldFromInstanceMethodRule

Problem: This instance method writes to static fields. This may cause problem with multiple instances in multithreaded applications.
* Severity: Medium, Confidence: High
* Target:   System.Void CVNBot.ListManager::BatchGetAllAdminsAndBots()
* Source:   debugging symbols unavailable, IL offset 0x010e
* Details:  The static field 'currentGetThreadMode', of type 'System.String'. is being set in an instance method.

Solution: Move initialization to the static constructor or ensure appropriate locking.
More info available at: https://github.com/spouliot/gendarme/wiki/Gendarme.Rules.Concurrency.WriteStaticFieldFromInstanceMethodRule(2.10)

I'm not entirely convinced this logic is actually working. At least in case of BatchGetAllAdminsAndBots it seems like the state is checked, but then it continues regardless.

We should see if there's a better way to deal with this.
@Krinkle Krinkle added the meta label Oct 15, 2017
@Krinkle Krinkle added this to the 1.22.0 milestone Oct 15, 2017
@Krinkle Krinkle self-assigned this Oct 15, 2017
@Krinkle Krinkle changed the title Evaluate whether userlist-thread lock is needed Evaluate whether userlist-thread lock is working Oct 15, 2017
Krinkle added a commit that referenced this issue Oct 16, 2017
@Krinkle
ListManager: Refactor userlist fetcher and use MediaWiki API
863973c 
* Remove locking and static state from userlist fetchers.

* Instead of using static members to pass data to the thread,
  use parameters instead (ParameterizedThreadStart).

* Remove the db locking. This was mostly a remnant from when
  the AddGroupToList method still used its own dedicated database
  connection (before baef0f3). However now that it uses its
  uses the main connection, we no longer need a lock around the
  whole batch (if anything, we very much don't want to have
  such a long-held lock). Instead, just let the for loop
  call addUserToList() for individual items as it does already
  which already ensures a lock on each individual update/insert.

* Get rid of the early returns that (tried to) prevent starting
  multiple userlist fetchers at the same time.

* Re-use local Thread variable in batchGetAll.

* Make the while-true loop sleep for 1ms instead of 0ms.

* Fix broken AddGroupToList() method:
  - Fix url to use HTTPS. Not sure whether it followed redirects.
  - Use the MediaWiki API instead of scraping Special:Listusers.
    This was broken in at least three ways:
    -  On en.wikipedia.org, the legend also contains a <ul>, so we're
       not going through the correct list.
    - Pattern wasn't matching because it assumed <a> to have only "href" and
       "title" attributes, whereas class="mw-userlink" is now set as well.
    - Bot was adding entries like "<bdi>Krinkle</bdi>" to the database,
      because at some point in the last year, user names in HTML output started
      to be wrapped by <bdi>.

Fixes #32.
Krinkle added a commit that referenced this issue Oct 16, 2017
@Krinkle
ListManager: Refactor userlist fetcher and use MediaWiki API
4bfe341 
* Remove locking and static state from userlist fetchers.

* Instead of using static members to pass data to the thread,
  use parameters instead (ParameterizedThreadStart).

* Remove the db locking. This was mostly a remnant from when
  the AddGroupToList method still used its own dedicated database
  connection (before baef0f3). However now that it uses its
  uses the main connection, we no longer need a lock around the
  whole batch (if anything, we very much don't want to have
  such a long-held lock). Instead, just let the for loop
  call addUserToList() for individual items as it does already
  which already ensures a lock on each individual update/insert.

* Get rid of the early returns that (tried to) prevent starting
  multiple userlist fetchers at the same time.

* Re-use local Thread variable in batchGetAll.

* Make the while-true loop sleep for 1ms instead of 0ms.

* Fix broken AddGroupToList() method:
  - Fix url to use HTTPS. Not sure whether it followed redirects.
  - Use the MediaWiki API instead of scraping Special:Listusers.
    This was broken in at least three ways:
    -  On en.wikipedia.org, the legend also contains a <ul>, so we're
       not going through the correct list.
    - Pattern wasn't matching because it assumed <a> to have only "href" and
       "title" attributes, whereas class="mw-userlink" is now set as well.
    - Bot was adding entries like "<bdi>Krinkle</bdi>" to the database,
      because at some point in the last year, user names in HTML output started
      to be wrapped by <bdi>.

Fixes #32.
Krinkle added a commit that referenced this issue Oct 16, 2017
@Krinkle
ListManager: Refactor userlist fetcher and use MediaWiki API
ca382db 
* Remove locking and static state from userlist fetchers.

* Instead of using static members to pass data to the thread,
  use parameters instead (ParameterizedThreadStart).

* Remove the db locking. This was mostly a remnant from when
  the AddGroupToList method still used its own dedicated database
  connection (before baef0f3). However now that it uses its
  uses the main connection, we no longer need a lock around the
  whole batch (if anything, we very much don't want to have
  such a long-held lock). Instead, just let the for loop
  call addUserToList() for individual items as it does already
  which already ensures a lock on each individual update/insert.

* Get rid of the early returns that (tried to) prevent starting
  multiple userlist fetchers at the same time.

* Re-use local Thread variable in batchGetAll.

* Make the while-true loop sleep for 1ms instead of 0ms.

* Fix broken AddGroupToList() method:
  - Fix url to use HTTPS. Not sure whether it followed redirects.
  - Use the MediaWiki API instead of scraping Special:Listusers.
    This was broken in at least three ways:
    -  On en.wikipedia.org, the legend also contains a <ul>, so we're
       not going through the correct list.
    - Pattern wasn't matching because it assumed <a> to have only "href" and
       "title" attributes, whereas class="mw-userlink" is now set as well.
    - Bot was adding entries like "<bdi>Krinkle</bdi>" to the database,
      because at some point in the last year, user names in HTML output started
      to be wrapped by <bdi>.

Fixes #32.
Krinkle added a commit that referenced this issue Oct 16, 2017
@Krinkle
ListManager: Refactor userlist fetcher and use MediaWiki API
bc17f4a 
* Remove locking and static state from userlist fetchers.

* Instead of using static members to pass data to the thread,
  use parameters instead (ParameterizedThreadStart).

* Remove the db locking. This was mostly a remnant from when
  the AddGroupToList method still used its own dedicated database
  connection (before baef0f3). However now that it uses its
  uses the main connection, we no longer need a lock around the
  whole batch (if anything, we very much don't want to have
  such a long-held lock). Instead, just let the for loop
  call addUserToList() for individual items as it does already
  which already ensures a lock on each individual update/insert.

* Get rid of the early returns that (tried to) prevent starting
  multiple userlist fetchers at the same time.

* Re-use local Thread variable in batchGetAll.

* Make the while-true loop sleep for 1ms instead of 0ms.

* Fix broken AddGroupToList() method:
  - Fix url to use HTTPS. Not sure whether it followed redirects.
  - Use the MediaWiki API instead of scraping Special:Listusers.
    This was broken in at least three ways:
    -  On en.wikipedia.org, the legend also contains a <ul>, so we're
       not going through the correct list.
    - Pattern wasn't matching because it assumed <a> to have only "href" and
       "title" attributes, whereas class="mw-userlink" is now set as well.
    - Bot was adding entries like "<bdi>Krinkle</bdi>" to the database,
      because at some point in the last year, user names in HTML output started
      to be wrapped by <bdi>.

Fixes #32.
@Krinkle Krinkle closed this as completed Oct 17, 2017
Krinkle added a commit that referenced this issue Oct 17, 2017
@Krinkle
ListManager: Refactor userlist fetcher and use MediaWiki API
0cfb4ed 
* Remove locking and static state from userlist fetchers.

* Instead of using static members to pass data to the thread,
  use parameters instead (ParameterizedThreadStart).

* Remove the db locking. This was mostly a remnant from when
  the AddGroupToList method still used its own dedicated database
  connection (before baef0f3). However now that it uses its
  uses the main connection, we no longer need a lock around the
  whole batch (if anything, we very much don't want to have
  such a long-held lock). Instead, just let the for loop
  call addUserToList() for individual items as it does already
  which already ensures a lock on each individual update/insert.

* Get rid of the early returns that (tried to) prevent starting
  multiple userlist fetchers at the same time.

* Re-use local Thread variable in batchGetAll.

* Make the while-true loop sleep for 1ms instead of 0ms.

* Fix broken AddGroupToList() method:
  - Fix url to use HTTPS. Not sure whether it followed redirects.
  - Use the MediaWiki API instead of scraping Special:Listusers.
    This was broken in at least three ways:
    -  On en.wikipedia.org, the legend also contains a <ul>, so we're
       not going through the correct list.
    - Pattern wasn't matching because it assumed <a> to have only "href" and
       "title" attributes, whereas class="mw-userlink" is now set as well.
    - Bot was adding entries like "<bdi>Krinkle</bdi>" to the database,
      because at some point in the last year, user names in HTML output started
      to be wrapped by <bdi>.

Fixes #32.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Krinkle Krinkle

Labels
meta
Milestone
1.22
Development

No branches or pull requests
1 participant
@Krinkle