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

add configurable auto-deletion of jobs #125

Merged
merged 1 commit into from
Jun 25, 2018

Conversation

anoymouserver
Copy link
Contributor

In some cases it might be necessary to access the KDSoapJob object after the work is done. Since the job deletes itself, this is currently not possible. I've implemented a new setter method to change this behavior (setAutoDelete). Maybe it could be also helpful to have a constructor parameter for this property. This would necessitate taking this parameter into all automatically generated subclasses.

@dfaure-kdab
Copy link
Member

Usually one would read from the job in a slot connected to the job's signal, and there you can still use the job, it hasn't been deleted yet [assuming this is all done in the same thread]. But yes if you keep the job instance around as a holder for that data, to be read much later (after the slot finishes executing) then I can see how auto-deletion is a problem.
Can you confirm that your use case isn't just using the job in the slot?

Assuming there's a valid use case, the patch looks good, it's just missing a unittest -- which would test that with autodelete off, deleting the job can actually be done manually, without crashes, later on.

@anoymouserver
Copy link
Contributor Author

Yes, I can confirm, that I need to read data from the job after the slot has finished. Basically you are right, that the slot should be used to get all data from the job. But I have created a special template method, that is able to process different derived KDSoapJobs. Since the slot doesn't know which derivative was started, I can't access the class specific members there.

So far I haven't created a unit test due to time constraints, but I can confirm that the patch runs without problems, since I already use it myself.

@dfaure-kdab
Copy link
Member

I can see how the template doesn't know the specific job subclass, but surely it ends up calling some code that does? Otherwise how would the data actually be fetched?
So it seems that "calling some code that knows" (e.g. via a virtual method or via some template code) isn't done directly but delayed, in your case? That's what I'm wondering about. If it's not delayed, then the job isn't deleted yet.

I mean: jobs autodelete using deleteLater(). So e.g. if you have two slots connecting to the job's finished signal, then they will both run before deletion happen. Or are you really going back to the event loop before reading the job's data?

@anoymouserver
Copy link
Contributor Author

In my application the code that fetches the data, is called delayed (when the slot is finished). Maybe this is not the best strategy and maybe it would be possible to change the whole structure, but in this case I'm only a part of a bigger software, that claims certain sequences. I have tried to find the best possible compromise.

So yes, in this case the event loop is active before the job's data is read.
Since my request only adds an optional behavior I don't see any danger for other users. Of course it's very convenient not taking care about the deletion of objects, but if it's required it would be good to have the opportunity to handle the object directly.

Copy link
Member

@dfaure-kdab dfaure-kdab left a comment

Choose a reason for hiding this comment

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

OK thanks for the use case explanation.
I suppose this can also be useful if the slot is read from another thread.

This is just missing a unittest, a changelog entry, and \since 1.8 near the new method. I'll add the last two in a followup commit.

Thanks for your contribution.

@dfaure-kdab dfaure-kdab merged commit 1f6e8d1 into KDAB:master Jun 25, 2018
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.

None yet

2 participants