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

Support synchronous methods #2

Closed
awidesky opened this issue Sep 30, 2022 · 2 comments
Closed

Support synchronous methods #2

awidesky opened this issue Sep 30, 2022 · 2 comments

Comments

@awidesky
Copy link
Contributor

There are some methods which only gives asynchronous option.
Like delete method in JMailTM class, or save method in Attachment class.

For me it looks like a good idea to give a synchronous version of this method.
It doesn't look a hard task to do since these methods have similar format(just some code snippet surrounded by new Thread(new Runnable() { ... }).start();).

So I'll be happy to do some contribution myself, but I'm not sure what is best option to achieve the goal.

1. Just make new synchronous method named like deleteNow

  • code would be like this :
    /**
     * (Synchronous) Deletes the Self Account
     */
    public void deleteNow(){
        try{
                IO.requestDELETE(baseUrl+"/accounts/"+id , bearerToken);
        }catch (Exception e){
                System.out.println("|EXCEPTION IGNORE | "+e);
        }
    }
    /**
     * (Asynchronous) Deletes the Self Account
     */
    public void delete(){
        new Thread(this::deleteNow).start();
    }
  • This would be a easy option, not effecting legacy code.
  • It may cause naming confusion, XX(Asynchronous) <-> XXNow(Synchronous) naming pattern doesn't match existing code like asyncXX(Asynchronous) <-> XX(Synchronous) (see fetchMessages)

2. Follow existing naming pattern

  • code would be like this :
    /**
     * (Synchronous) Deletes the Self Account
     */
    public void delete(){
        try{
                IO.requestDELETE(baseUrl+"/accounts/"+id , bearerToken);
        }catch (Exception e){
                System.out.println("|EXCEPTION IGNORE | "+e);
        }
    }
    /**
     * (Asynchronous) Deletes the Self Account
     */
    public void asyncDelete(){
        new Thread(this::deleteNow).start();
    }
  • There will be no naming confusion to users.
  • This does effect legacy code, making many existing code/project broken.

3. Make delete method returns Future object

  • Make a Thread Pool, and submit all asynchronous tasks(deleting account, saving attachments, fetching mails, etc..) to it.
  • In this case, submit method will return a Future object.
  • So it will be easy to wait for the thread, and WorkCallback class will be easily changed to Future<Boolean>
  • It is likely not to effect legacy code, since only return type of the method is changed. But, some rare case(e.g. method referencing like jmailtmObjectList.stream().foreach(JMailTM::delete)) will result an error.
  • A Thread Pool is used, so we should be careful to avoid bottleneck.(but Thread Pool looks like a better option than making new thread every time calling a method)

What would be the best option?

@shivam1608 shivam1608 reopened this Oct 1, 2022
@shivam1608
Copy link
Owner

shivam1608 commented Oct 1, 2022

sorry i accidentally clicked closed lol
well i was planning to shift to FutureTask & Thread pool executor as you said its not a good practice to create new thread all time
but then it will not exit the program (in case someone making a script to do some specify task) so i made it that way so that the developer using this library can use Thread pool in their code using synchronous methods (which i missed for few methods thanks for creating an issue) provided in the lib that's why i would like to stick to legacy methods.
still you can make a pull request if you want.

@awidesky
Copy link
Contributor Author

awidesky commented Oct 1, 2022

Maybe could use shutdown hook to terminate Thread Pool in the end of the program, but It more looks like a punt than a solution.
I'll choose option 1, and add some synchronous versions of some methods.
I will make pull request when work's done :D

@awidesky awidesky mentioned this issue Oct 1, 2022
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

No branches or pull requests

2 participants