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

Fix double initialization of Database class #4482

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

ArturKnopik
Copy link
Contributor

Fix double initialization of Database class, Database now become true singleton

Pull Request Prelude

  • I have followed [proper The Forgotten Server code styling][code].
  • I have read and understood the [contribution guidelines][cont] before making this PR.
  • I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

Issues addressed:

@ranisalt ranisalt merged commit 56d1bc8 into otland:master Jun 8, 2023
@ArturKnopik ArturKnopik deleted the databaseSingleton branch June 8, 2023 17:11
@nekiro
Copy link
Member

nekiro commented Jun 8, 2023

DatabaseTasks operates in its own thread, currently with this change you access the singleton from two different threads simultaneously. This code now is probably not thread safe, thus broken unless I'm missing something here
database has mutex inside :)

@ArturKnopik
Copy link
Contributor Author

ArturKnopik commented Jun 8, 2023

Good point, need to be checked
Will be nice to add some job that will check memory leaks and synchronization
@edit
mutex confirmed

@kygov
Copy link
Contributor

kygov commented Jun 9, 2023

@nekiro
Copy link
Member

nekiro commented Jun 9, 2023

So database tasks are no longer asynchronously?

https://github.com/otland/forgottenserver/blob/master/src/luascript.cpp#L4184 https://github.com/otland/forgottenserver/blob/master/src/luascript.cpp#L4224

They are, using the same instance doesn't make it synchronous, but in special cases where simultaneous access could occur it will not cause undefined behaviour. If you mean it will not execute at the same time, it never did, mysql has to queue these actions anyway for obvious reasons. Asynchronous in this case means don't block main thread. I'm not exactly sure how does it behave with long queries for example in DatabaseTasks, cause this would block the thread (due to mutex locking) and then hang the main thread, which is something we don't want, so you might be onto something here

@kygov
Copy link
Contributor

kygov commented Jun 9, 2023

AFAIK previous it had its own connection to mysql and you could execute two queries in same time if they were not to same table (then mysql would queue it).
I will check it later.

@nekiro
Copy link
Member

nekiro commented Jun 9, 2023

AFAIK previous it had its own connection to mysql and you could execute two queries in same time if they were not to same table (then mysql would queue it). I will check it later.

Yup, this PR is most likely a revert xD

ArturKnopik pushed a commit to ArturKnopik/forgottenserver that referenced this pull request Jun 9, 2023
nekiro pushed a commit that referenced this pull request Jun 9, 2023
This reverts commit 56d1bc8.

Co-authored-by: AKnopik PC <krecikondexin@gmail.con>
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.

5 participants