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

Switched to AsyncKeyedLock #877

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MarkCiliaVincenti
Copy link

This PR will reduce memory allocations and memory usage.

@MarkCiliaVincenti
Copy link
Author

@openbullet :)

@openbullet
Copy link
Owner

Hey sorry I didn't have time to look into this yet, will do soon ^^'

@MarkCiliaVincenti
Copy link
Author

Hey sorry I didn't have time to look into this yet, will do soon ^^'

gentle reminder :)

@meinname
Copy link
Collaborator

meinname commented Apr 29, 2023

@openbullet asked me to try it.
And i noticed that jobs don't start as they don't load proxies. - without proxies it seems to be ok. (tho i did no extensive test without being able to use proxies)
(Tested on native)

@MarkCiliaVincenti
Copy link
Author

@openbullet asked me to try it. And i noticed that jobs don't start as they don't load proxies. - without proxies it seems to be ok. (tho i did no extensive test without being able to use proxies) (Tested on native)

Hi,

What would be the easiest way to reproduce this issue please so that I could maybe debug? Or would you kindly be able to debug and tell me where the issue lies please?

@MarkCiliaVincenti
Copy link
Author

@meinname

@meinname
Copy link
Collaborator

Simply try to start a job with proxies.
It wont start

@MarkCiliaVincenti
Copy link
Author

@meinname I'm going to need more info to replicate this issue please. Also can you try again please?

@MarkCiliaVincenti
Copy link
Author

@meinname please let me know. I've also updated to the latest version of the library with performance improvements.

@meinname
Copy link
Collaborator

meinname commented Jan 23, 2024

Still. When starting a Job with proxies off it seems to work.
But as soon as i use proxies (tested from OB2 Proxy Manager and from File) a Job with a simple config only remains at Starting...
(only did a quick test this time on native Client on Windows)

@MarkCiliaVincenti
Copy link
Author

Would you be so kind as to dumb it down into steps with screenshots to reproduce? And can you confirm that without the PR it does not give an issue?

@meinname
Copy link
Collaborator

meinname commented Jan 23, 2024

I don't know hot to dumb it down more - but i try

First i check out ob2 master to a new directory and publish it to a new directory.
Then i start the native client and create a new config - basically anything you want. - i just used one as simple as that one:

BLOCK:RandomInteger
  => VAR @randomIntegerOutput
ENDBLOCK

BLOCK:Keycheck
  KEYCHAIN SUCCESS OR
    INTKEY @randomIntegerOutput GreaterThanOrEqualTo 9
  KEYCHAIN FAIL OR
    INTKEY @randomIntegerOutput LessThan 9
ENDBLOCK

and add a few simple proxies on ob2 proxy tab to a new group.
As they aren't used with this config, i just add some like 127.0.0.1:9000 127.0.0.1:9001 .... or real ones or whatever.

I create a new multirun job and
select the config. - set bots to 10 - proxy mode to on - wordlist either use one via file or just use infinite
And then go to the job and start it.
It works. - job runs and you get successes and fails

then do the same but with the PR merged
And then you can see that after pressing start ob2 does nothing. Beside job status is and stays on "Starting..."
Edit: oh, and if i do the same and switch proxy mode to off it works

@MarkCiliaVincenti
Copy link
Author

That looks very helpful actually @meinname, thanks I'll look at it probably over the weekend.

@github-actions github-actions bot added the merge-conflict This PR has a merge conflict that needs to be resolved before it can be merged label Jul 1, 2024
Copy link

github-actions bot commented Jul 1, 2024

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@Gauvino
Copy link
Contributor

Gauvino commented Jul 10, 2024

@MarkCiliaVincenti Hello, first thank for your contribution, could please resolve the merge conflict to get this reviewed ?

@MarkCiliaVincenti
Copy link
Author

I took a look at the updated code and I tried my best to see how I could get the AsyncKeyedLock library to work with OpenBullet2 but I don't understand what that LoliCodeBlockInstance is.

More importantly for you guys is that the current AsyncLocker class is not just very inefficient but worse than that has obvious race conditions.

You will need to refactor your code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-conflict This PR has a merge conflict that needs to be resolved before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants