search

Tectu / malloy

A cross-platform C++20 library providing embeddable server & client components for HTTP and WebSocket.
BSD 3-Clause "New" or "Revised" License
66 stars 8 forks source link

make controller_run_result movable and expose it as start_result #108

Closed 0x00002a closed 2 years ago

0x00002a commented 2 years ago

Related to #106 and #107.

5874a5ac847f9cba1a1dd475fda79268e76cd2ac implemented part of this but there is a subtle bug in it which shouldn't exist in this one. Feel free to close and/or cherry pick the move changes as the rest may be unwanted while #107 is open, but make sure to include 6456a16988d46db62ded44d728854d6665b7f890 as I accidentally removed the work guard when merging it with my local changes.

Also of note is the message in 6456a16988d46db62ded44d728854d6665b7f890, in which I discovered why we need a work guard for the io context.

Tectu commented 2 years ago

Well, that was some unfortunate timing :p

5874a5a implemented part of this but there is a subtle bug in it which shouldn't exist in this one.

Could you be more specific? I must be missing something obvious... Would love to hear about it.

0x00002a commented 2 years ago

5874a5a implemented part of this but there is a subtle bug in it which shouldn't exist in this one.

Could you be more specific? I must be missing something obvious... Would love to hear about it.

The threads spawned by controller_run_result capture this and then access the m_io_context through it. The issue here is that the function passed to the thread ctor only runs after the thread is spawned, meaning if the code to move the object is executed first, as is possible in the test case which does it on the next line (and which only happened for me when a debugger wasn't attached, that was fun), then the m_io_context being accessed through the this pointer was the one that had already been moved from and now pointed to nullptr. All in all I'm very glad I wrote a test case or I would never have caught that one :p

Also it doesn't check in run() for if the object has been moved from, meaning it could result in UB via nullptr deference there. In this I've thrown an exception but I'm not sure if that should be an assert

Tectu commented 2 years ago

The threads spawned by controller_run_result capture this and then access the m_io_context through it. The issue here is that the function passed to the thread ctor only runs after the thread is spawned, meaning if the code to move the object is executed first, as is possible in the test case which does it on the next line (and which only happened for me when a debugger wasn't attached, that was fun), then the m_io_context being accessed through the this pointer was the one that had already been moved from and now pointed to nullptr. All in all I'm very glad I wrote a test case or I would never have caught that one :p

Ouch... that one would definitely have made for some very, very unpleasant nights of debugging down the road. I am very glad that you caught that one... - Eagle eyes 😉

Also it doesn't check in run() for if the object has been moved from, meaning it could result in UB via nullptr deference there. In this I've thrown an exception but I'm not sure if that should be an assert

Now that was just a stupid oversight - should have taken the courtesy to get some sleep before committing this - sorry!

Not sure whether we should:

0x00002a commented 2 years ago

Well I don't really have much more time to spend on this, and prolly won't for a few days unfortunately so it might be best to roll back 5874a5ac847f9cba1a1dd475fda79268e76cd2ac since it isn't safe to move the object anyway and then merge this when it implements whatever is decided for #107, since otherwise its likely going to make the solution a breaking change.

I'm pretty sure I can just resolve conflicts by hand on github though by just not accepting 5874a5ac847f9cba1a1dd475fda79268e76cd2ac when merging

Tectu commented 2 years ago

I'm a bit tight on time as well. I will revert my commit which introduced the bug you discovered/mentioned so that main and circle back to this later.

Thank you for all the time you invest into this. It's greatly appreciated :)

0x00002a commented 2 years ago

I've done the renames suggested in #107. I also renamed the header files and such, not sure if routing_context is the final name we want to go with, but start_result is now session instead and is exposed by both the client controller and server routing context

Tectu commented 2 years ago

Nice work! Seems like we have some demo fixing to do.