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

Improvements to concurrent tasks #1694

Merged
merged 26 commits into from Dec 11, 2023
Merged

Conversation

Trial97
Copy link
Member

@Trial97 Trial97 commented Oct 6, 2023

I did get a crash:

Env:

  • latest develop (a7bdfb5)
  • Qt 6.5.3
  • Number of concurrent tasks 24
  • Number of concurrent downloads 6

I have high hopes that this may solve some other issues regarding concurrent tasks

Here are some of the fixed issues:

Once this is released we should notify:

Trial97 and others added 12 commits July 14, 2023 21:07
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Co-authored-by: seth <getchoo@tuta.io>
Signed-off-by: Alexandru Ionut Tripon <alexandru.tripon97@gmail.com>
 into fail_concurrent_task

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
 into fail_concurrent_task

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97 Trial97 added bug Something isn't working simple change changelog:fixed A PR that appears under "Fixed" in the changelog labels Oct 6, 2023
@Trial97 Trial97 added this to the 8.0 milestone Oct 6, 2023
@Trial97 Trial97 marked this pull request as draft October 6, 2023 22:14
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97 Trial97 removed this from the 8.0 milestone Oct 13, 2023
@Trial97
Copy link
Member Author

Trial97 commented Oct 13, 2023

Removed from milestone until I have a clear idea for this

 into concurrent

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
…er into concurrent

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97 Trial97 added the needs-testing PRs that should be tested a bit more before being merged label Oct 18, 2023
@Trial97
Copy link
Member Author

Trial97 commented Oct 18, 2023

This is the successor of #1364.
Right now this is not stable (tests may still fail on actions).
I need to test most tasks(similar to the old PR)

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97
Copy link
Member Author

Trial97 commented Oct 20, 2023

Now that I magically solved the action tests, here is a list with all the stuff I will test in order to see if this works or not:(with asan)

ConcurrentTask

  • Create a Custom instance
  • Create an Import instance:
    • from URL
    • from Curseforge URL
    • Curseforge
    • Modrinth
    • Prism
  • Create an AtLauncher instance
  • Create a Curseforge instance
    • with blocked mods
  • Create a Ftb Legacy instance:
    • public
    • Third Party
    • Private
  • Create a Ftb App Import instance
  • Create a Modrinth instance
  • Create a Technic instance
  • try to abort the task when it's running for the add instance action
  • Update mods:
    • with metadata
    • without metadata
    • dependency
  • Export pack:
    • Prism
    • Modrinth
    • Curseforge
  • Download:
    • mods
    • mods with dependency
    • resource packs
    • shaders
    • textures
  • That resources are correctly displayed(mods/resource packs/shaders/textures)

SequentialTask

  • Imgur Uplad with album
  • Skin upload
  • update mods
  • check for mods dependencies

NetJob

  • all download dialogs:
    • Curseforge
    • Modrinth
  • all mod packs
  • the mod/mod pack/resource description is loaded correctly
  • the meta (versions and loaders)
  • the asset download (the stuff that is prompted right before you play the game)
  • all loaders work(with libraries and stuff):
    • vanilla
    • forge
    • neoforge
    • fabric
    • quilt
    • litleloader
  • the online resource logo loads correctly
  • the news is loaded
  • the translation is loading
  • the curseforge modpack import works (from website)
  • the curseforge mod import works (from website)
  • managed pack page works:
    • Curseforge
    • Modrinth
    • and version change for both providers
    • and version refresh for both providers
  • Imgur upload

extra

  • about dialog
  • account list page works
  • try to move through the resource lists rapidly

MultipleOptionsTask is not used so there is no test for it

** This list is not final, it's just a list from a quick search through code for ConccurentTask stuff. If I missed any test case let me know.

  • I will reset this list if I make changes to the PR(meaning I need to test all cases again)

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97 Trial97 marked this pull request as ready for review October 22, 2023 17:08
@Trial97
Copy link
Member Author

Trial97 commented Oct 22, 2023

Tested all that complicated stuff and it all works now.
With that, this is ready to review and test.
As I modified the ConncurentTask stuff I expect at least one other person(or more) will test this PR.
And a small question for somebody with more qt knowledge:
If I define a QSharedPointer, then connect a signal from that pointer to lambda and capture sharedPointer through value; Will that pointer be deleted automatically, or do I need some code that calls deleteLater or disconnect on that pointer?

@Trial97 Trial97 added this to the 8.1 milestone Oct 22, 2023
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97
Copy link
Member Author

Trial97 commented Oct 26, 2023

Now this was tested with asan enabled so we should be good on the memory side

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Copy link
Member

@TayouVR TayouVR left a comment

Choose a reason for hiding this comment

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

I am not super confident in my testing, but I was not able to create a crash or otherwise bad behaviour... so it looks good to me.
More people please test!

@Scrumplex Scrumplex added the backport release-8.x Backport PR automatically label Dec 11, 2023
@Scrumplex Scrumplex merged commit 9467b11 into PrismLauncher:develop Dec 11, 2023
32 checks passed
Copy link
Contributor

Backport failed because this pull request contains merge commits. You can either backport this pull request manually, or configure the action to skip merge commits.

Scrumplex added a commit that referenced this pull request Dec 11, 2023
@Scrumplex Scrumplex added manual backport PRs that have been backported manually and removed backport release-8.x Backport PR automatically labels Dec 11, 2023
@Trial97 Trial97 mentioned this pull request Feb 2, 2024
1 task
LunaisLazier pushed a commit to LunaisLazier/ShatteredPrism that referenced this pull request Feb 9, 2024
@Scrumplex Scrumplex changed the title Improvements to concurrent task Improvements to concurrent tasks Mar 3, 2024
@Scrumplex Scrumplex added changelog:changed A PR that appears under "Changed" in the changelog and removed changelog:fixed A PR that appears under "Fixed" in the changelog labels Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment