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 asynchronous execution #109

Merged
merged 24 commits into from
Jun 12, 2019
Merged

Add asynchronous execution #109

merged 24 commits into from
Jun 12, 2019

Conversation

iamed2
Copy link
Collaborator

@iamed2 iamed2 commented May 30, 2019

Adds support for asynchronous execution using https://www.postgresql.org/docs/10/libpq-async.html

TODO:

  • A few more docstrings
  • General docs
  • More tests for atypical behaviour (e.g., closing a connection before a query finishes)
  • Test with task.sticky = false
    • Currently deadlocks (I think during cancellation but I'm not sure) when using @par, will wait another release to be sure it's not a bad interaction with the uv event loop

@iamed2 iamed2 requested a review from oxinabox May 30, 2019 22:49
src/asyncresults.jl Outdated Show resolved Hide resolved
src/asyncresults.jl Outdated Show resolved Hide resolved
src/asyncresults.jl Outdated Show resolved Hide resolved
src/asyncresults.jl Outdated Show resolved Hide resolved
src/asyncresults.jl Outdated Show resolved Hide resolved
@iamed2 iamed2 force-pushed the ed/async-execute branch from e6f14ab to 93c0c5d Compare June 3, 2019 19:51
src/connections.jl Outdated Show resolved Hide resolved
@JuliaDatabases JuliaDatabases deleted a comment from oxinabox Jun 4, 2019
@JuliaDatabases JuliaDatabases deleted a comment from oxinabox Jun 4, 2019
@JuliaDatabases JuliaDatabases deleted a comment from oxinabox Jun 4, 2019
@JuliaDatabases JuliaDatabases deleted a comment from oxinabox Jun 4, 2019
@iamed2
Copy link
Collaborator Author

iamed2 commented Jun 4, 2019

(deleted the duplicate submissions)

src/asyncresults.jl Outdated Show resolved Hide resolved
src/asyncresults.jl Outdated Show resolved Hide resolved
Co-Authored-By: Lyndon White <oxinabox@ucc.asn.au>
src/asyncresults.jl Outdated Show resolved Hide resolved
end
end

iserror(async_result::AsyncResult) = Base.istaskfailed(async_result.result_task)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to open an upstream issue to get this exported?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

src/asyncresults.jl Outdated Show resolved Hide resolved
src/asyncresults.jl Outdated Show resolved Hide resolved
iamed2 and others added 2 commits June 11, 2019 13:42
Co-Authored-By: Lyndon White <oxinabox@ucc.asn.au>
Co-Authored-By: Lyndon White <oxinabox@ucc.asn.au>
Base.islocked(conn::Connection) = islocked(conn.lock)
Base.lock(conn::Connection) = acquire(conn.semaphore)
Base.unlock(conn::Connection) = release(conn.semaphore)
Base.islocked(conn::Connection) = conn.semaphore.curr_cnt >= conn.semaphore.sem_size
Copy link
Member

Choose a reason for hiding this comment

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

We should raise an issue up stream that semaphore does not implement islocked

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

finally
unlock(conn)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

why did yo unroll this rather than using lock(f, conn.lock)?
Is it because Semaphore's don't implement that pattern?
if so we should open an issue about it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, lock(f, the_lock) is restricted to ::AbstractLock, even though It probably shouldn't be. I mentioned that one a draft PR I made that no one looked at.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@oxinabox
Copy link
Member

Do Connection and AsyncResults ever have seperate lives?
Or is in 1 Connection has at most 1 linked AsyncResult and then it iis deallocated.

@iamed2
Copy link
Collaborator Author

iamed2 commented Jun 11, 2019

Connections can only have one AsyncResult at a time, but can have many AsyncResults over their lifetime.

@oxinabox
Copy link
Member

Ok, I am now in-principle happy with this PR.
Most of what is outsanding to my mind is links to upstream issues about missing things.
And the fixing of tests etc.

docs/src/index.md Outdated Show resolved Hide resolved
@iamed2 iamed2 force-pushed the ed/async-execute branch from d18f6db to 69a7b61 Compare June 11, 2019 21:12
@iamed2 iamed2 merged commit 1c03dc2 into master Jun 12, 2019
@omus omus deleted the ed/async-execute branch January 28, 2020 15:32
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.

3 participants