non-blocking versions of file open/stat #256

weissi · 2018-03-29T18:59:01Z

addresses #185. This is a first draft to discuss the API.

Motivation:

Opening a file, seeking it or querying its size (or other information)
is blocking on UNIX so NonBlockingFileIO should support that too.

Modifications:

Added a method to NonBlockingFileIO which lets the user open a file
without blocking the calling thread.

Result:

Less blocking is good :)

weissi · 2018-03-29T19:24:30Z

test failure should hopefully be addressed by #257

weissi · 2018-03-29T19:24:38Z

@swift-nio-bot please test

weissi · 2018-03-30T09:20:54Z

@swift-nio-bot please test

tomerd · 2018-03-30T14:36:03Z

@swift-nio-bot test this please
@wrissi you need to ask right :)

Lukasa

One minor question, otherwise looks good.

Lukasa · 2018-04-03T08:22:09Z

Sources/NIO/NonBlockingFileIO.swift

+ guard case shouldRun = BlockingIOThreadPool.WorkItemState.active else {
+ p.fail(error: ChannelError.ioOnClosedChannel)
+ return
+ }


Should we have a convenience wrapper around submit that adds this boilerplate?

proposed an API for that now.

Lukasa · 2018-04-03T09:24:58Z

Sources/NIO/BlockingIOThreadPool.swift

@@ -75,6 +75,8 @@ public final class BlockingIOThreadPool {

 /// Submit a `WorkItem` to process.
 ///
+ /// - note: This is a low-level method, in most cases the `submit` method should be used.


This should presumably be runIfActive, not submit.

fixed, thanks

Lukasa

LGTM, let's let @normanmaurer review as well.

normanmaurer · 2018-04-03T19:21:49Z

Sources/NIO/BlockingIOThreadPool.swift

+ let promise: EventLoopPromise<T> = eventLoop.newPromise()
+ self.submit { shouldRun in
+ guard case shouldRun = BlockingIOThreadPool.WorkItemState.active else {
+ promise.fail(error: ChannelError.ioOnClosedChannel)


IMHO using an ChannelError here is a bit odd as the BlockingIOThreadPool has nothing to do with a Channel at all and we also have no reference to the Channel.

@normanmaurer yes, that is indeed very odd. However that is what the other methods do so I copied it. Happy to fix but then we're slightly inconsistent...

hmm ok... We should have spotted this before :( I guess we need to fix this in a next major

@normanmaurer well, we could just be different in this particular method. That wouldn't be breaking API as it's a new API. It would just behave slightly differently than the read apis.

normanmaurer · 2018-04-03T19:22:15Z

Sources/NIO/BlockingIOThreadPool.swift

+ /// - eventLoop: The `EventLoop` to create the returned `EventLoopFuture` on.
+ /// - body: The closure which performs some blocking work to be done on the thread pool.
+ /// - returns: An `EventLoopFuture` fulfilled with the result (or error) of the passed closure.
+ public func runIfActive<T>(eventLoop: EventLoop, _ body: @escaping () throws -> T) -> EventLoopFuture<T> {


Should we maybe better just take the EventLoopPromise<T> directly and not the EventLoop ?

hmm, unconvinced as there are more things that can go wrong if we ask the user to give us a promise instead of just making one. What's the benefit of taking a promise instead of the EventLoop?

The major benefit is that the user controls where the future fires. That's basically it though.

@Lukasa but that's why I take the EventLoop. I tell the user that the returned future will fired on the EventLoop passed in there.

Yup, basically isomorphic here.

@weissi for me taking a EventLoop looks more like running something on the EventLoop, which is not the case here. Also taking an EventLoopPromise is more consistent with what we do for ChannelOutboundInvoker imho.

also @Lukasa

yeah, let @Lukasa be the tie breaker.

Ok, I think we should take a promise: it's more consistent with the other interfaces.

Lukasa

I'm happy, waiting on @normanmaurer.

normanmaurer · 2018-04-09T18:14:27Z

Sources/NIO/BlockingIOThreadPool.swift

+ /// - promise: The `EventLoopPromise` that will be fulfilled when the block finished running (or failed).
+ /// - body: The closure which performs some blocking work to be done on the thread pool.
+ /// - returns: The `EventLoopFuture` of `promise` fulfilled with the result (or error) of the passed closure.
+ public func runIfActive<T>(promise: EventLoopPromise<T>, _ body: @escaping () throws -> T) -> EventLoopFuture<T> {


@weissi @Lukasa sorry but I need to correct my previous suggestion here... whenever we take a promise we use Void as return type. So maybe we should do the same here as well ??

Yeah, probably. It's likely to be non-obvious that the Future returned here is the one corresponding to the passed-in promise.

@Lukasa and @normanmaurer can we please go back to taking the EventLoop? It's important that we return an EventLoopFuture as the next thing you want to do is clearly work with that file. The taking-the-promise approach is just a performance improvement if you might not care about the result of the operation (usually for EventLoopFuture<Void>s). For opening files however we absolutely do care about the return value. So we will need a promise&future here and I still think we should take the EventLoop

@Lukasa & @normanmaurer updated this guy

Lukasa

My previous +1 still applies.

normanmaurer · 2018-04-10T17:12:42Z

@weissi please rebase, address conflicts and merge

weissi · 2018-04-11T15:24:36Z

@swift-nio-bot test this please

tomerd · 2018-04-11T16:53:59Z

@swift-nio-bot test this please

Motivation: Opening a file, seeking it or querying its size (or other information) is blocking on UNIX so `NonBlockingFileIO` should support that too. Modifications: Added a method to `NonBlockingFileIO` which lets the user open a file without blocking the calling thread. Result: Less blocking is good :)

weissi · 2018-04-18T13:17:15Z

@normanmaurer / @Lukasa I'll merge this when the tests go green as we'll release a 1.5.0 very soon, okay?

Lukasa · 2018-04-18T13:38:12Z

WFM.

normanmaurer · 2018-04-18T13:38:37Z

same... WFM

weissi requested review from Lukasa and normanmaurer March 29, 2018 19:01

weissi force-pushed the jw-non-blocking-file-opening branch from 16d9d6c to ecacadf Compare March 29, 2018 19:01

weissi force-pushed the jw-non-blocking-file-opening branch from ecacadf to af9d966 Compare March 30, 2018 14:47

weissi added the semver/minor Adds new public API. label Mar 30, 2018

Lukasa requested changes Apr 3, 2018

View reviewed changes

Lukasa added this to the 1.4.0 milestone Apr 3, 2018

weissi force-pushed the jw-non-blocking-file-opening branch from af9d966 to 252339b Compare April 3, 2018 09:09

weissi added the discussion label Apr 3, 2018

Lukasa requested changes Apr 3, 2018

View reviewed changes

weissi force-pushed the jw-non-blocking-file-opening branch from 252339b to f0d35ad Compare April 3, 2018 10:48

Lukasa approved these changes Apr 3, 2018

View reviewed changes

normanmaurer requested changes Apr 3, 2018

View reviewed changes

weissi force-pushed the jw-non-blocking-file-opening branch 2 times, most recently from 60f6164 to 64e7553 Compare April 6, 2018 10:20

Lukasa approved these changes Apr 6, 2018

View reviewed changes

normanmaurer reviewed Apr 9, 2018

View reviewed changes

weissi removed the discussion label Apr 10, 2018

weissi force-pushed the jw-non-blocking-file-opening branch from 64e7553 to 7df910d Compare April 10, 2018 14:03

Lukasa approved these changes Apr 10, 2018

View reviewed changes

normanmaurer approved these changes Apr 10, 2018

View reviewed changes

weissi force-pushed the jw-non-blocking-file-opening branch from 505ad73 to 388d576 Compare April 11, 2018 14:16

Lukasa modified the milestones: 1.4.0, 1.5.0 Apr 11, 2018

weissi force-pushed the jw-non-blocking-file-opening branch from 8e764e9 to 3b1a8e9 Compare April 18, 2018 13:16

Merge branch 'master' into jw-non-blocking-file-opening

49e3943

weissi merged commit 5327cb1 into apple:master Apr 18, 2018

weissi deleted the jw-non-blocking-file-opening branch April 18, 2018 16:56

Lukasa mentioned this pull request Apr 19, 2018

No non-blocking file-open operation #185

Closed

non-blocking versions of file open/stat #256

non-blocking versions of file open/stat #256

Conversation

weissi commented Mar 29, 2018 • edited Loading

weissi commented Mar 29, 2018

weissi commented Mar 29, 2018

weissi commented Mar 30, 2018

tomerd commented Mar 30, 2018

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

normanmaurer Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

normanmaurer commented Apr 10, 2018

weissi commented Apr 11, 2018

tomerd commented Apr 11, 2018

weissi commented Apr 18, 2018

Lukasa commented Apr 18, 2018

normanmaurer commented Apr 18, 2018

weissi commented Mar 29, 2018 •

edited

Loading

normanmaurer Apr 9, 2018 •

edited

Loading