massive URI-handling / IPC server re-work #1023

Diapolo commented at 12:45 PM on April 1, 2012: none

What I try to achieve here:

harden the URI handling / IPC server (i.e. buffer length checks use of IMPLEMENT_RANDOMIZE_STACK for the ipcThread)
log and handle boost interprocess exceptions where possible
make URI handling more usable / robust on Windows (and not worse on other platforms - needs tests on Linux OSes)

What I achieved (running and verified on Windows):

it's possible to start the client via URI click, if no message_queue file exists
if one exists the clicked URI is sent to the queue file, but doesn't start the client (call to try_send() in bitcoin.cpp succeeds and we exit there - currently no check if the client is running - ideas are welcome), the URI is processed after manually starting the client and is not lost
on Windows it's now possible to use 2 client instances in parallel and the first started one receives clicked URIs, if I close one of the instances the one left will process URIs
URI length is limited to 255 chars as message queue message size is 256 chars max.

What I could not achieve:

handle some special cases i.e. a BitcoinURI file get's created by a user and is placed in the boost_interprocess folder -> results in the GUI freeze bug, as the file permissions differ from what boost would set, when creating the mq file -> that causes a boost deadlock (https://svn.boost.org/trac/boost/ticket/6745)

Some details: The message queue is not removed anymore when closing the client, as this causes massive handling problems on Windows with 2 instances and my former approach of stale mq detection. First try is always to open the mq file for a re-use. If this fails a new mq is created.

This needs boost 1.49 with a small edit in boost/interprocess/detail/tmp_dir_helpers.hpp see: https://svn.boost.org/trac/boost/ticket/5392#comment:24

There is currently no need for any hard monkey-patches like #986.

in src/qt/bitcoin.cpp:None in 8b4659d3b9 outdated

 155 | +            catch (interprocess::interprocess_exception &ex) {
 156 | +                // don't log the "file not found" exception, because that's normal for
 157 | +                // the first start of the first instance
 158 | +                if (ex.get_error_code() != interprocess::not_found_error)
 159 | +                {
 160 | +                    printf("boost interprocess exception #%d: %s\n", ex.get_error_code(), ex.what());

Diapolo commented at 12:45 PM on April 1, 2012:

Is there a possibility to display a message box before the GUI is initialized?

laanwj commented at 7:15 AM on April 2, 2012:

Before the bitcoin GUI is initialized, in GUI code, in the main thread: yes you can use the QMessageBox::critical static function
Before Qt and translation services are initialized: no

In this place here you can't.

gavinandresen commented at 6:35 PM on April 2, 2012: contributor

Please don't open pull requests until you think your code is 100% perfect and tested and ready to go.

Before then, you can ask people to look at your code or help out using your github repository.

laanwj commented at 12:29 PM on April 7, 2012: member

These might be interesting as an alternative to boost::interprocess (and flakey shared memory queues in general):

http://doc.qt.nokia.com/4.7-snapshot/qlocalserver.html http://doc.qt.nokia.com/4.7-snapshot/qlocalsocket.html

They provide a named socket (unix socket on unix, pipe on windows) to communicate between processes on the same machine.

Alternatively, boost also supports unix sockets and windows named pipes, however, the difference is not abstracted away like in the Qt implementation so will need #ifdefs.

luke-jr commented at 1:51 PM on April 10, 2012: member

Rebasing required.

Diapolo commented at 9:01 PM on April 10, 2012: none

Rebased and re-worked a little. I need someone to test this ;).

Needs boost 1.49 with a small edit in boost/interprocess/detail/tmp_dir_helpers.hpp see: https://svn.boost.org/trac/boost/ticket/5392#comment:24

There is currently no need for any hard monkey-patches like #986.

gavinandresen commented at 1:22 AM on April 11, 2012: contributor

NACK-- not worth requiring boost 1.49 for this, and "a small edit in boost" scares the pants off me.

Diapolo commented at 6:23 AM on April 11, 2012: none

The small edit is uncommenting 3 lines of code that are already IN the tmp_dir_helpers.hpp ... I really would like to know what your problem with a boost update is? If you could explain it a little it makes it easier for me to understand, thanks.

laanwj commented at 7:13 AM on April 11, 2012: member

With the amount of trouble it has given us, I think we can safely conclude that boost::interprocess is not ready for production use yet until upstream gets their act together. Also it seems aimed at much more complex use-cases such as sharing memory and objects instead of signaling simple lines of text between processes.

When I have some time I'll try coming up with a QLocalServer/QLocalSocket based implementation.

gavinandresen commented at 5:32 PM on April 11, 2012: contributor

RE: what's the trouble with requiring everybody upgrade to boost 1.49: boost is probably the hardest of our dependencies to get compiled and working, and somebody running an older version of Linux or OSX that spent a day getting boost 1.46 compiled and working properly to compile bitcoind isn't going to be happy if we tell them "you need 1.49 now, because we need that version to fix a bug in URI handling on Windows."

They may not care about bitcoin-qt at all and certainly don't care about Windows....

laanwj commented at 5:45 PM on April 11, 2012: member

Gavin: I don't completely follow this reasoning. URL handling (and any usage of boost::interprocess) is limited to bitcoin-qt, if we required boost 1.49 for bitcoin-qt for Windows doesn't mean everyone has to upgrade. The rest of the code can remain backwards compatible. Also, even for bitcoin-qt, the older boost::interprocess works fine in Linux.

Diapolo commented at 9:28 PM on April 11, 2012: none

It's to easy to say Linux / OSX users may not care about Bitcoin-Qt at all and certainly don't care about Windows. How many Windows users download the client and how many use the GUI version contra how many Linux users are out there? For example I never used bitcoind as Windows users like GUIs and that's a fact :). I could say I don't care about Linux users that want to compile bitcoind, but I don't do this :D.

But I'm fine with my work currently not beeing used ... even if no one ever tested it ;).

luke-jr commented at 2:05 PM on May 27, 2012: member

wtf is with 1e39376? you just committed conflicts!

Diapolo commented at 2:27 PM on May 27, 2012: none

Dunno how that happened, I normally don't include rebase-conflicts ;), should work again. Sorry @luke-jr!

massive URI-handling / IPC server re-work 6e2fb937f7

in src/qt/bitcoin.cpp:None in 6e2fb937f7 outdated

 123 |  
 124 |      // Do this early as we don't want to bother initializing if we are just calling IPC
 125 |      for (int i = 1; i < argc; i++)
 126 |      {
 127 | -        if (boost::algorithm::istarts_with(argv[i], "bitcoin:"))
 128 | +        // limit length of parsed URIs to max. size of message queue messages

TheBlueMatt commented at 12:53 PM on June 10, 2012:

This is redundant, boost already deals with this for us, see message_queue.hpp:501 (in 1.49) "//Check if buffer is smaller than maximum allowed"

Diapolo commented at 3:13 PM on June 10, 2012:

ACK, this gets removed!

in src/qt/bitcoin.cpp:None in 6e2fb937f7 outdated

 127 | -        if (boost::algorithm::istarts_with(argv[i], "bitcoin:"))
 128 | +        // limit length of parsed URIs to max. size of message queue messages
 129 | +        if (strlen(argv[i]) <= IPC_MQ_MAX_MESSAGE_SIZE && boost::algorithm::istarts_with(argv[i], "bitcoin:"))
 130 |          {
 131 | -            const char *strURI = argv[i];
 132 | +            std::string strURI = argv[i];

TheBlueMatt commented at 12:54 PM on June 10, 2012:

Is there a reason to use a std:;string instead of a const char*, seems like std::string needlessly copies the data to yet another buffer? (and then copies it back in 2 lines where you call c_str()

laanwj commented at 2:35 PM on June 10, 2012:

I think it's more consistent to use C++ string handling everywhere instead of strlen() and friends (performance is not an argument here). In most implementations, c_str doesn't perform a copy, but it simply returns a pointer to the internal buffer. Though it'd be better to use .data() if you don't rely on it being zero-terminated like in this case.

laanwj commented at 2:36 PM on June 10, 2012:

I do wonder why strlen is still used in the line above this, though. I guess the consistency argument doesn't hold either :-)

Diapolo commented at 3:16 PM on June 10, 2012:

As above line will get the strlen removed your argument counts :). My intention was to avoid pure char * usage as much as possible and use std::string for this.

in src/qt/qtipcserver.cpp:None in 6e2fb937f7 outdated

  61 | +            uiInterface.ThreadSafeHandleURI(std::string(buffer, nSize));
  62 |              Sleep(1000);
  63 |          }
  64 | +        else
  65 | +            /** needs to be here to stop this thread from utilizing 100% of a CPU core */
  66 | +            Sleep(100);

TheBlueMatt commented at 12:58 PM on June 10, 2012:

Why convert to try_receive instead of timed_receive? With timed_receive we can handle URIs instantly without any messy Sleeps and without eating CPU?

Diapolo commented at 3:23 PM on June 10, 2012:

To be honest, I really don't recall, why I changed this ... it's no problem to revert to a timed_receive() :D.

in src/qt/qtipcserver.cpp:None in 6e2fb937f7 outdated

  17 |  using namespace boost;
  18 | -using namespace std;
  19 |  
  20 | -void ipcShutdown()
  21 | +/** global state definition */
  22 | +ipcState globalIpcState = IPC_NOT_INITIALIZED;

TheBlueMatt commented at 12:59 PM on June 10, 2012:

Is there a reason for this? I see it only set but never read, which seems pointless?

in src/qt/qtipcserver.cpp:None in 6e2fb937f7 outdated

 135 | +        {
 136 | +            if (!fInitCalledAfterRecovery)
 137 | +            {
 138 | +                printf("ipcInit - trying to open current message queue...\n");
 139 |  
 140 | -        // Make sure only one bitcoin instance is listening

TheBlueMatt commented at 1:00 PM on June 10, 2012:

Is there a reason this is no longer needed?

Diapolo commented at 3:26 PM on June 10, 2012:

Quite a few devs were telling me it needs to be possible to use multiple instances in parallel AND as I wrote above this pulls doesn't remove a message_queue, so this was removed intentionally.

TheBlueMatt commented at 4:55 PM on June 10, 2012:

These few lines dont make it impossible to use multiple instances in parallel, in fact they make it more sane. Without these lines, multiple instances get URIs distributed between them, which is...non-intuitive IMHO. With these lines they simply all go to the most recent running version.

in src/qt/qtipcserver.cpp:None in 6e2fb937f7 outdated

  72 | -        }
  73 |      }
  74 | -    ipcShutdown();
  75 | +
  76 | +    /** cleanup allocated memory and set global IPC state to not initialized */
  77 | +    delete mq;

TheBlueMatt commented at 1:02 PM on June 10, 2012:

Im assuming it does, but this does call message_queue::remove, correct?

Diapolo commented at 3:25 PM on June 10, 2012:

It does NOT call remove(), which is one of the bigger changes in this pull. We leave the mq where it is and try to re-use it.

TheBlueMatt commented at 4:58 PM on June 10, 2012:

If we leave it, does Bitcoin-Qt open properly? Would it not try to send to the mq, send properly, and then exit (when opening with a URI after previously closing)? Making anyone open Bitcoin-Qt several times until the mq fills up?

in src/qt/qtipcserver.cpp:None in 6e2fb937f7 outdated

  22 | +ipcState globalIpcState = IPC_NOT_INITIALIZED;
  23 | +
  24 | +void ipcThread(void* pArg)
  25 |  {
  26 | -    message_queue::remove(BITCOINURI_QUEUE_NAME);
  27 | +    IMPLEMENT_RANDOMIZE_STACK(ipcThread(pArg));

TheBlueMatt commented at 1:02 PM on June 10, 2012:

ACK, we really should do this.

in src/qt/bitcoin.cpp:None in 6e2fb937f7 outdated

 132 | +            std::string strURI = argv[i];
 133 |              try {
 134 | -                boost::interprocess::message_queue mq(boost::interprocess::open_only, BITCOINURI_QUEUE_NAME);
 135 | -                if(mq.try_send(strURI, strlen(strURI), 0))
 136 | +                boost::interprocess::message_queue mq(boost::interprocess::open_only, IPC_MQ_NAME);
 137 | +                if (mq.try_send(strURI.c_str(), strURI.length(), 0))

Diapolo commented at 3:20 PM on June 10, 2012:

@laanwj You suggested to use .data() instead of .c_str() for this here, but I'm not sure, why we don't need it to it be zero-terminated, can you explain please :).

update 1 5fc2f75cf8

in src/qt/bitcoin.cpp:None in 6e2fb937f7 outdated

 281 | @@ -271,14 +282,17 @@ int main(int argc, char *argv[])
 282 |                  // Check for URI in argv
 283 |                  for (int i = 1; i < argc; i++)
 284 |                  {
 285 | -                    if (boost::algorithm::istarts_with(argv[i], "bitcoin:"))
 286 | +                    // only bother with this if IPC is initialized
 287 | +                    if (globalIpcState == IPC_INITIALIZED && strlen(argv[i]) <= IPC_MQ_MAX_MESSAGE_SIZE && boost::algorithm::istarts_with(argv[i], "bitcoin:"))

Diapolo commented at 3:21 PM on June 10, 2012:

@TheBlueMatt globalIpcState is used here, to avoid errors, if ipcInit fails for some reason.

TheBlueMatt commented at 5:02 PM on June 10, 2012:

Ah, well then would it not be cleaner to return a value from ipcInit instead of adding a global state variable?

Diapolo commented at 3:59 PM on June 10, 2012: none

Updated to reflect the last suggestions from the discussion, all commits will be merged after this get's final (if it get's final ^^), so I used NO speaking commit message!

Diapolo commented at 10:15 AM on July 6, 2012: none

I will cherry-pick some of the changes and open a new pull.

Diapolo closed this on Jul 6, 2012

suprnurd referenced this in commit 4e6bb6a375 on Dec 5, 2017

Bushstar referenced this in commit 19f576835f on Oct 14, 2019

lateminer referenced this in commit 509d63526b on Oct 30, 2019

lateminer referenced this in commit a761323178 on Oct 30, 2019

DrahtBot locked this on Sep 8, 2021