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

Diapolo · 2012-04-01T12:45:15Z

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.

Diapolo · 2012-04-01T12:45:56Z

src/qt/bitcoin.cpp

+                // the first start of the first instance
+                if (ex.get_error_code() != interprocess::not_found_error)
+                {
+                    printf("boost interprocess exception #%d: %s\n", ex.get_error_code(), ex.what());


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

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 · 2012-04-02T18:35:49Z

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 · 2012-04-07T12:29:52Z

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 · 2012-04-10T13:51:44Z

Rebasing required.

Diapolo · 2012-04-10T21:01:55Z

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 · 2012-04-11T01:22:22Z

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

Diapolo · 2012-04-11T06:23:45Z

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 · 2012-04-11T07:13:38Z

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 · 2012-04-11T17:32:36Z

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 · 2012-04-11T17:45:34Z

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 · 2012-04-11T21:28:01Z

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 · 2012-05-27T14:05:39Z

wtf is with 1e39376? you just committed conflicts!

Diapolo · 2012-05-27T14:27:31Z

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

TheBlueMatt · 2012-06-10T12:53:46Z

src/qt/bitcoin.cpp


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


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"

ACK, this gets removed!

Diapolo · 2012-06-10T15:59:13Z

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 · 2012-07-06T10:15:50Z

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

1c1488c Revert "Restore lint whitespace check" (Bushstar) Pull request description: Tree-SHA512: c40cea97bece30df9052f9b284ca236185c5b220c012b65ea5211d7808bd52ae98c84850fcd30f11a92c36d8c6204879b851e5ec94b19a0bc590ebc751c85ac7

8c1c6f2 [Travis] Lower timeout for the full test suite (warrows) Pull request description: Set the travis build timeout for the longest job to 21mn 40sec. Set the build timeout for the other jobs back to 33 mn and 20 sec. This should avoid global 50 mn timeout on the longest job and avoid having to restart other jobs needlessly. ACKs for top commit: random-zebra: ACK 8c1c6f2 furszy: ACK 8c1c6f2 Tree-SHA512: fac84a3789805150647c791c49c1691708350f220fdb1993662318bcf784bb150986c3f7553f5946895671cb0c82a07344df49a6184cd47a662d1302c42825de

88c0d54 Update .travis.yml (Warrows) Pull request description: Fix a mistake from bitcoin#1023 preventing the full test build to use its custom timeout. ACKs for top commit: random-zebra: utACK 88c0d54 furszy: utACK 88c0d54 Tree-SHA512: 3c424360db7be0dc9a9f773c3f0b3cd301119f45cbca0ddc6aef4b7abb3a189fb679a0740df2643c3298d7253f2b1097e28ad0da705e8c39d6a1ed318744c1a2

Diapolo reviewed Apr 1, 2012
View reviewed changes

Diapolo mentioned this pull request Jun 10, 2012

Windows URI Support #1437

Merged

massive URI-handling / IPC server re-work

6e2fb93

TheBlueMatt reviewed Jun 10, 2012
View reviewed changes

update 1

5fc2f75

Diapolo closed this Jul 6, 2012

suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this pull request Dec 5, 2017

Fixed incorrect rpcgovernance error/usage message (bitcoin#1023)

4e6bb6a

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Diapolo commented Apr 1, 2012

Diapolo Apr 1, 2012

laanwj Apr 2, 2012

gavinandresen commented Apr 2, 2012

laanwj commented Apr 7, 2012

luke-jr commented Apr 10, 2012

Diapolo commented Apr 10, 2012

gavinandresen commented Apr 11, 2012

Diapolo commented Apr 11, 2012

laanwj commented Apr 11, 2012

gavinandresen commented Apr 11, 2012

laanwj commented Apr 11, 2012

Diapolo commented Apr 11, 2012

luke-jr commented May 27, 2012

Diapolo commented May 27, 2012

TheBlueMatt Jun 10, 2012

Diapolo Jun 10, 2012

Diapolo commented Jun 10, 2012

Diapolo commented Jul 6, 2012

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

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

Conversation

Diapolo commented Apr 1, 2012

Diapolo Apr 1, 2012

Choose a reason for hiding this comment

laanwj Apr 2, 2012

Choose a reason for hiding this comment

gavinandresen commented Apr 2, 2012

laanwj commented Apr 7, 2012

luke-jr commented Apr 10, 2012

Diapolo commented Apr 10, 2012

gavinandresen commented Apr 11, 2012

Diapolo commented Apr 11, 2012

laanwj commented Apr 11, 2012

gavinandresen commented Apr 11, 2012

laanwj commented Apr 11, 2012

Diapolo commented Apr 11, 2012

luke-jr commented May 27, 2012

Diapolo commented May 27, 2012

TheBlueMatt Jun 10, 2012

Choose a reason for hiding this comment

Diapolo Jun 10, 2012

Choose a reason for hiding this comment

Diapolo commented Jun 10, 2012

Diapolo commented Jul 6, 2012