That's exactly what it was. The main interface was being freed after the client that it came from.
An exception in an IOCP promise is a confusing way to find out that I messed up cleaning up some objects. Is there some way to tell if an event loop (or in my case EzRpcClient since I'm not using event loops directly yet) still has outstanding work to do? Thanks for your help! Joe On Saturday, April 20, 2019 at 10:15:25 AM UTC-7, Kenton Varda wrote: > > Hi Joe, > > Hmm, I'm not sure. It's helpful to read the comments that you removed, > which can be found here: > > > https://github.com/capnproto/capnproto/blob/d68cffe544e8479f3cb01a88d8a547eff8782d71/c++/src/kj/async-win32.c++#L57-L91 > > IoPromiseAdapter is waiting for a specific event to complete. The > completion is indicated by GetQueuedCompletionStatus() on the IOCP > returning the OVERLAPPED structure associated with this IoPromiseAdapter. > We cannot allow the IoPromiseAdapter to be destroyed until that time, > because the operating system may still be operating on that structure. > > The purpose of the while() loop calling port.waitIocp() is to repeatedly > call GetQueuedCompletionStatus() until we get our event, at which point the > destructor can safely complete. > > When CancelIoEx() produces ERROR_INVALID_HANDLE, this means that the > handle has already been closed (probably, recently, due to destructors > running in the wrong order). I believe closing the handle should implicitly > cancel all I/O, but we still have to wait for the IOCP to produce the event > completions before we can destroy the OVERLAPPED structure. > > What do you mean by "this call chokes on an invalid handle and throws an > exception"? What part of the call throws exactly? What's the stack trace? > waitIocp() shouldn't perform any new operations on the now-closed handle; > it should only wait for old operations to complete. > > Is it possible that the Win32IocpEventPort *itself* has also already been > destroyed, and so waitIocp() fails because the IOCP handle is invalid? If > that's the case, this indicates a bug in your code: you've allowed the > event loop to be shut down while you still had events in-flight. You'll > need to make sure you destroy all promises and Cap'n Proto objects before > you shut down the event loop. > > If that's not it, could you produce a small self-contained test that > demonstrates the problem, for us to debug? > > -Kenton > > On Sat, Apr 20, 2019 at 9:24 AM Joe Ludwig <[email protected] > <javascript:>> wrote: > >> Hi Kenton, >> >> I think I may be running into some of the cleanup complication you're >> implying. During that, I think I'm seeing a straight-up bug, though. The >> snippet of code below is from async-win32.c++, just with the comment blocks >> removed. >> >> Win32IocpEventPort::~IoPromiseAdapter() { >> if (handle != INVALID_HANDLE_VALUE) { >> if (!CancelIoEx(handle, this)) { >> DWORD error = GetLastError(); >> >> >> if (error != ERROR_NOT_FOUND && error != ERROR_INVALID_HANDLE) { >> KJ_FAIL_WIN32("CancelIoEx()", error, handle); >> } >> // if error was ERROR_INVALID_HANDLE, the handle will be != >> INVALID_HANDLE_VALUE >> } >> >> >> while (handle != INVALID_HANDLE_VALUE) { >> port.waitIocp(INFINITE); // this call chokes on an invalid >> handle and throws an exception >> } >> } >> } >> >> >> Here's one possible fix: >> ~IoPromiseAdapter() { >> if (handle != INVALID_HANDLE_VALUE) { >> if (!CancelIoEx(handle, this)) { >> DWORD error = GetLastError(); >> >> >> if( error == ERROR_INVALID_HANDLE ) { >> // we know the handle has gone invalid, so just forget our >> handle >> handle = INVALID_HANDLE_VALUE; >> } >> else if (error != ERROR_NOT_FOUND ) { >> KJ_FAIL_WIN32("CancelIoEx()", error, handle); >> } >> } >> >> >> while (handle != INVALID_HANDLE_VALUE) { >> port.waitIocp(INFINITE); >> } >> } >> } >> >> >> >> If it would help, I can express that in the form of a pull request. >> >> >> Joe >> >> >> On Friday, April 19, 2019 at 10:03:32 AM UTC-7, Kenton Varda wrote: >>> >>> Hi Joe, >>> >>> Take a look at the doc comments for thisCap(): >>> >>> inline Capability::Client thisCap(); >>> // Get a capability pointing to this object, much like the `this` >>> keyword. >>> // >>> // The effect of this method is undefined if: >>> // - No capability client has been created pointing to this object. >>> (This is always the case in >>> // the server's constructor.) >>> // - The capability client pointing at this object has been destroyed. >>> (This is always the case >>> // in the server's destructor.) >>> // - Multiple capability clients have been created around the same >>> server (possible if the server >>> // is refcounted, which is not recommended since the client itself >>> provides refcounting). >>> >>> Your code is hitting the first bullet point -- you're trying to call >>> thisCap() before any capability client has been created. >>> >>> What you really want to do here is take advantage of the fact that >>> capability clients are copyable using reference counting. You can create a >>> client, and then keep a copy for yourself, like so: >>> >>> auto server = kj::heap<MyTypeImpl>(...); >>> auto& serverRef = *server; >>> MyType::Client capability = kj::mv(server); >>> >>> // Send a ref to the client. >>> context.getResults().setApp(capability); >>> >>> // Also keep one for ourself, along with a reference to the >>> implementation >>> myServers.add(ClientRefPair { client, serverRef }); >>> >>> By keeping a `Client` that points to your server object, you ensure that >>> the refcount remains non-zero so the object stays alive. As shown above, >>> you can separately keep a reference or pointer to the underlying server >>> object if you need to be able to access it's C++ interface directly. >>> >>> Note that your server's destructor is only called after all references >>> -- yours, and remote references -- are dropped. If you need to force your >>> server object to be destroyed at a specific time, then you'll need to do >>> one of the following: >>> >>> - Use a layer of indirection where your server object wraps another >>> object that can be nulled out later, such that all the RPC methods start >>> throwing exceptions once nulled. >>> - Wrap the Client object in a membrane (capnp/membrane.h) which you >>> later revoke. >>> >>> -Kenton >>> >>> On Thu, Apr 18, 2019 at 10:00 PM Joe Ludwig <[email protected]> wrote: >>> >>>> I have a really simple RPC protocol: >>>> interface AvServer >>>> { >>>> createApp @0 ( name: Text ) -> ( app: AvApp ); >>>> } >>>> >>>> >>>> interface AvApp >>>> { >>>> name @0 () -> ( name: Text ); >>>> >>>> >>>> destroy @1 (); >>>> } >>>> >>>> >>>> I implement that on the server with two concrete classes: >>>> >>>> class AvServerImpl final : public AvServer::Server >>>> class CAardvarkApp final: public AvApp::Server >>>> >>>> >>>> >>>> And then when the client calls createApp I want to create an AvApp, >>>> return a reference to it, and keep ownership of the app itself in some >>>> datastructure on the server: >>>> >>>> ::kj::Promise<void> AvServerImpl::createApp( CreateAppContext context >>>> ) >>>> { >>>> // "works" >>>> context.getResults().setApp( kj::heap<CAardvarkApp>( context.getParams >>>> ().getName() ) ); >>>> >>>> >>>> // crashes because "thisHook" is null >>>> auto pApp = kj::heap<CAardvarkApp>( context.getParams().getName() ); >>>> context.getResults().setApp( pApp->GetClient() ); // GetClient() is >>>> just a call to thisCap() >>>> m_listApps.push_back( std::move( pApp ) ); >>>> >>>> >>>> return kj::READY_NOW; >>>> } >>>> >>>> >>>> The first approach there works, but the AvServerImpl class doesn't keep >>>> any kind of record of the new CAardvarkApp object, so it won't be able to >>>> do anything with the app outside of a call from this one particular client. >>>> >>>> What I'm attempting to do in the second approach is create an object on >>>> the heap and set it in the results, but keep ownership of it in the list. >>>> I >>>> don't get that far, though, because thisCap crashes because thisHook >>>> returns nullptr. >>>> >>>> I think I must be missing something really fundamental about how all >>>> this is supposed to work. Am I required to create a unique foo::Server >>>> object for each incoming call that refers to a single logical object? If I >>>> have to pass ownership to the results in order to return values, it seems >>>> like something along that line will be called for. That doesn't sound >>>> right, though, since I'd have to manage an unbounded number of those >>>> objects as requests come in from different clients. >>>> >>>> Help? :) >>>> >>>> >>>> Joe >>>> >>>> -- >>>> You received this message because you are subscribed to the Google >>>> Groups "Cap'n Proto" group. >>>> To unsubscribe from this group and stop receiving emails from it, send >>>> an email to [email protected]. >>>> Visit this group at https://groups.google.com/group/capnproto. >>>> >>> -- >> You received this message because you are subscribed to the Google Groups >> "Cap'n Proto" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected] <javascript:>. >> Visit this group at https://groups.google.com/group/capnproto. >> > -- You received this message because you are subscribed to the Google Groups "Cap'n Proto" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. Visit this group at https://groups.google.com/group/capnproto.
