New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

slight change in redis example leads to access violation #855

Closed
Extrawurst opened this Issue Oct 2, 2014 · 27 comments

Comments

Projects
None yet
3 participants
@Extrawurst
Contributor

Extrawurst commented Oct 2, 2014

module app;

import vibe.vibe;

shared static this()
{
    /* open a redis server locally to run these tests
     * Windows download link: https://github.com/MSOpenTech/redis/tree/2.8/bin/release
     * Linux: use "yum install redis" on RHEL or "apt-get install redis" on Debian-like
    */
    RedisClient redis;
    try redis = new RedisClient();
    catch (Exception) {
        logInfo("Failed to connect to local Redis server. Skipping test.");
        return;
    }
    /+{
        ....
    }+/
    RedisSubscriber sub = new RedisSubscriber(redis);
    import std.datetime;

    assert(!sub.isListening);
    sub.listen((string channel, string msg){
        logInfo("LISTEN Recv Channel: %s, Message: %s", channel.to!string, msg.to!string);
        logInfo("LISTEN Recv Time: %s", Clock.currTime().toString());
    });

    sleep(1.seconds);
    assert(sub.isListening);
    sub.subscribe("SomeChannel");

    logInfo("PUBLISH Sent: %s", Clock.currTime().toString());
    redis.getDatabase(0).publish("SomeChannel", "Messageeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee");
    sub.unsubscribe("SomeChannel");
    auto stopped = sub.bstop();
    logInfo("LISTEN Stopped: %s", stopped.to!string);
    assert(!sub.isListening);
    redis.getDatabase(0).publish("SomeChannel", "Messageeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee");
    sleep(1.seconds);
    logInfo("Redis Test Succeeded.");
}

i simply reduced the redis example code to just test the pubsub part and that leads to an access violation in the current rc1 tag version:

Task terminated with unhandled exception: Access Violation
object.Error: Access Violation
Error executing command run: Program exited with code 1

interestingly nothing crashes when the example is run as a whole with the out-commented code part

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 2, 2014

Contributor

run with -vvvv reveals this:

e:\_docs\_bitbucket\redistest>redistest.exe -vvvv
[02490F80:00000000 dia] libevent version: 2.0.20-stable
[02490F80:00000000 dia] libevent is using win32 for events.
[02490F80:02494B00 ERR] Task terminated with unhandled exception: Access Violation
[02490F80:02494B00 ERR] Test failed: Access Violation
[02490F80:02494B00 dia] Full error: object.Error: Access Violation
[02490F80:02494B00 dia] ----------------
[02490F80:02494B00 dia] 0x004168ED in void vibe.db.redis.redis.RedisSubscriber.blisten(void delegate(immutable(char)[], immutable(char)[]), core.time.Duration) at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\db\redis\redis.d(571)
[02490F80:02494B00 dia] 0x0041700D in D4vibe2db5redis5redis15RedisSubscriber6listenMFDFAyaAyaZvS4core4time8DurationZS4vibe4core4task4Task9__lambda3MFZv at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\db\redis\redis.d(680)
[02490F80:02494B00 dia] 0x0040F728 in D4vibe4core4core27__T16makeTaskFuncInfoTDFZvZ16makeTaskFuncInfoFNbDFZvZS4vibe4core4core12TaskFuncInfo12callDelegateFPS4vibe4core4core12TaskFuncInfoZv at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\core\core.d(478)
[02490F80:02494B00 dia] 0x0041F86C in void vibe.core.core.CoreTask.run() at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\core\core.d(959)
[02490F80:02494B00 dia] 0x004FBD7D in void core.thread.Fiber.run()
[02490F80:02494B00 dia] 0xFFFFFFFF
[02490F80:02494B00 dia] 0x77311E8B in RtlInitializeContext
core.exception.InvalidMemoryOperationError
Contributor

Extrawurst commented Oct 2, 2014

run with -vvvv reveals this:

e:\_docs\_bitbucket\redistest>redistest.exe -vvvv
[02490F80:00000000 dia] libevent version: 2.0.20-stable
[02490F80:00000000 dia] libevent is using win32 for events.
[02490F80:02494B00 ERR] Task terminated with unhandled exception: Access Violation
[02490F80:02494B00 ERR] Test failed: Access Violation
[02490F80:02494B00 dia] Full error: object.Error: Access Violation
[02490F80:02494B00 dia] ----------------
[02490F80:02494B00 dia] 0x004168ED in void vibe.db.redis.redis.RedisSubscriber.blisten(void delegate(immutable(char)[], immutable(char)[]), core.time.Duration) at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\db\redis\redis.d(571)
[02490F80:02494B00 dia] 0x0041700D in D4vibe2db5redis5redis15RedisSubscriber6listenMFDFAyaAyaZvS4core4time8DurationZS4vibe4core4task4Task9__lambda3MFZv at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\db\redis\redis.d(680)
[02490F80:02494B00 dia] 0x0040F728 in D4vibe4core4core27__T16makeTaskFuncInfoTDFZvZ16makeTaskFuncInfoFNbDFZvZS4vibe4core4core12TaskFuncInfo12callDelegateFPS4vibe4core4core12TaskFuncInfoZv at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\core\core.d(478)
[02490F80:02494B00 dia] 0x0041F86C in void vibe.core.core.CoreTask.run() at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\core\core.d(959)
[02490F80:02494B00 dia] 0x004FBD7D in void core.thread.Fiber.run()
[02490F80:02494B00 dia] 0xFFFFFFFF
[02490F80:02494B00 dia] 0x77311E8B in RtlInitializeContext
core.exception.InvalidMemoryOperationError
@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 3, 2014

Contributor

Could you test it with this? #815

I also had these issues so I rewrote the subscriber code completely

Contributor

etcimon commented Oct 3, 2014

Could you test it with this? #815

I also had these issues so I rewrote the subscriber code completely

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 3, 2014

Contributor

@etcimon yes it still happens with your changes:

e:\_docs\_bitbucket\redistest>redistest.exe -vvvv
[00BC0F80:00000000 dia] libevent version: 2.0.20-stable
[00BC0F80:00000000 dia] libevent is using win32 for events.
[00BC0F80:00BC4900 ERR] Task terminated with unhandled exception: Access Violation
[00BC0F80:00BC4B00 ERR] Task terminated with unhandled exception: Access Violation
[00BC0F80:00BC4B00 ERR] Test failed: Access Violation
[00BC0F80:00BC4B00 dia] Full error: object.Error: Access Violation
[00BC0F80:00BC4B00 dia] ----------------
[00BC0F80:00BC4B00 dia] 0x00415D02 in void vibe.db.redis.redis.RedisSubscriber.blisten(void delegate(immutable(char)[], immutable(char)[]), core.time.Duration).void __lambda7() at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\db\redis\redis.d(703)
[00BC0F80:00BC4B00 dia] 0x0040DB10 in D4vibe4core4core27__T16makeTaskFuncInfoTDFZvZ16makeTaskFuncInfoFNbDFZvZS4vibe4core4core12TaskFuncInfo12callDelegateFPS4vibe4core4core12TaskFuncInfoZv at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\core\core.d(478)
[00BC0F80:00BC4B00 dia] 0x00422634 in void vibe.core.core.CoreTask.run() at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\core\core.d(959)
[00BC0F80:00BC4B00 dia] 0x004FEF0D in void core.thread.Fiber.run()
[00BC0F80:00BC4B00 dia] 0xFFFFFFFF
[00BC0F80:00BC4B00 dia] 0x77311E8B in RtlInitializeContext
core.exception.InvalidMemoryOperationError
Contributor

Extrawurst commented Oct 3, 2014

@etcimon yes it still happens with your changes:

e:\_docs\_bitbucket\redistest>redistest.exe -vvvv
[00BC0F80:00000000 dia] libevent version: 2.0.20-stable
[00BC0F80:00000000 dia] libevent is using win32 for events.
[00BC0F80:00BC4900 ERR] Task terminated with unhandled exception: Access Violation
[00BC0F80:00BC4B00 ERR] Task terminated with unhandled exception: Access Violation
[00BC0F80:00BC4B00 ERR] Test failed: Access Violation
[00BC0F80:00BC4B00 dia] Full error: object.Error: Access Violation
[00BC0F80:00BC4B00 dia] ----------------
[00BC0F80:00BC4B00 dia] 0x00415D02 in void vibe.db.redis.redis.RedisSubscriber.blisten(void delegate(immutable(char)[], immutable(char)[]), core.time.Duration).void __lambda7() at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\db\redis\redis.d(703)
[00BC0F80:00BC4B00 dia] 0x0040DB10 in D4vibe4core4core27__T16makeTaskFuncInfoTDFZvZ16makeTaskFuncInfoFNbDFZvZS4vibe4core4core12TaskFuncInfo12callDelegateFPS4vibe4core4core12TaskFuncInfoZv at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\core\core.d(478)
[00BC0F80:00BC4B00 dia] 0x00422634 in void vibe.core.core.CoreTask.run() at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\core\core.d(959)
[00BC0F80:00BC4B00 dia] 0x004FEF0D in void core.thread.Fiber.run()
[00BC0F80:00BC4B00 dia] 0xFFFFFFFF
[00BC0F80:00BC4B00 dia] 0x77311E8B in RtlInitializeContext
core.exception.InvalidMemoryOperationError
@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 3, 2014

Contributor

This crash was introduced after the 0.7.21-alpha.4 tag, because there the code still works without crashing

Contributor

Extrawurst commented Oct 3, 2014

This crash was introduced after the 0.7.21-alpha.4 tag, because there the code still works without crashing

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 3, 2014

Contributor

For some reason m_lockedConnection.conn in blisten is null even though init() is called and should be created using the connectionPool...

Contributor

Extrawurst commented Oct 3, 2014

For some reason m_lockedConnection.conn in blisten is null even though init() is called and should be created using the connectionPool...

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 3, 2014

Contributor

I think we would need to add something like this at the bottom of init:

if (!m_lockedConnection.conn || !m_lockedConnection.conn.connected) {
    try m_lockedConnection.conn = connectTCP(m_lockedConnection.m_host, m_lockedConnection.m_port);
    catch (Exception e) {
        throw new Exception(format("Failed to connect to Redis server at %s:%s.", m_lockedConnection.m_host, m_lockedConnection.m_port), __FILE__, __LINE__, e);
    }

    m_lockedConnection.setAuth(m_client.m_authPassword); 
    m_lockedConnection.setDB(m_client.m_selectedDB);
}

And on line 703 change the while to:

if (!m_stop && m_lockedConnection.conn && m_lockedConnection.conn.waitForData(100.msecs)) {

I'm not working today but if you have time you could try this out.

Contributor

etcimon commented Oct 3, 2014

I think we would need to add something like this at the bottom of init:

if (!m_lockedConnection.conn || !m_lockedConnection.conn.connected) {
    try m_lockedConnection.conn = connectTCP(m_lockedConnection.m_host, m_lockedConnection.m_port);
    catch (Exception e) {
        throw new Exception(format("Failed to connect to Redis server at %s:%s.", m_lockedConnection.m_host, m_lockedConnection.m_port), __FILE__, __LINE__, e);
    }

    m_lockedConnection.setAuth(m_client.m_authPassword); 
    m_lockedConnection.setDB(m_client.m_selectedDB);
}

And on line 703 change the while to:

if (!m_stop && m_lockedConnection.conn && m_lockedConnection.conn.waitForData(100.msecs)) {

I'm not working today but if you have time you could try this out.

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 7, 2014

Contributor

your proposed changes lead to a slightly different output but still a crash:

e:\_docs\_bitbucket\redistest>redistest.exe -vvvv
[024E0F80:00000000 dia] libevent version: 2.0.20-stable
[024E0F80:00000000 dia] libevent is using win32 for events.
[024E0F80:024E4900 CRITICAL] Task terminated with uncaught exception: Failed to connect to Redis server at 127.0.0.1:6379.
[024E0F80:024E4B00 ERR] Test failed: Failed to start listening, timeout of 5 seconds expired
[024E0F80:024E4B00 dia] Full error: object.Exception@C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\db\redis\redis.d(771): Failed to start listening, timeout of 5 seconds expired
[024E0F80:024E4B00 dia] ----------------
[024E0F80:024E4B00 dia] 0x004FC293 in pure @safe void std.exception.bailOut(immutable(char)[], uint, const(char[]))
[024E0F80:024E4B00 dia] 0x004157BB in vibe.core.task.Task vibe.db.redis.redis.RedisSubscriber.listen(void delegate(immutable(char)[], immutable(char)[]), core.time.Duration) at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\db\redis\redis.d(771)
[024E0F80:024E4B00 dia] 0x004030DE in void app.runTest() at e:\_docs\_bitbucket\redistest\source\app.d(79)
[024E0F80:024E4B00 dia] 0x00403379 in void app.main().__lambda1() at e:\_docs\_bitbucket\redistest\source\app.d(98)
[024E0F80:024E4B00 dia] 0x0040DB10 in D4vibe4core4core27__T16makeTaskFuncInfoTDFZvZ16makeTaskFuncInfoFNbDFZvZS4vibe4core4core12TaskFuncInfo12callDelegateFPS4vibe4core4core12TaskFuncInfoZv at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\core\core.d(478)
[024E0F80:024E4B00 dia] 0x004228B4 in void vibe.core.core.CoreTask.run() at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\core\core.d(959)
[024E0F80:024E4B00 dia] 0x004FEFE9 in void core.thread.Fiber.run()
core.exception.InvalidMemoryOperationError

note that there is actually not running any redis, but nevertheless it should not crash

Contributor

Extrawurst commented Oct 7, 2014

your proposed changes lead to a slightly different output but still a crash:

e:\_docs\_bitbucket\redistest>redistest.exe -vvvv
[024E0F80:00000000 dia] libevent version: 2.0.20-stable
[024E0F80:00000000 dia] libevent is using win32 for events.
[024E0F80:024E4900 CRITICAL] Task terminated with uncaught exception: Failed to connect to Redis server at 127.0.0.1:6379.
[024E0F80:024E4B00 ERR] Test failed: Failed to start listening, timeout of 5 seconds expired
[024E0F80:024E4B00 dia] Full error: object.Exception@C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\db\redis\redis.d(771): Failed to start listening, timeout of 5 seconds expired
[024E0F80:024E4B00 dia] ----------------
[024E0F80:024E4B00 dia] 0x004FC293 in pure @safe void std.exception.bailOut(immutable(char)[], uint, const(char[]))
[024E0F80:024E4B00 dia] 0x004157BB in vibe.core.task.Task vibe.db.redis.redis.RedisSubscriber.listen(void delegate(immutable(char)[], immutable(char)[]), core.time.Duration) at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\db\redis\redis.d(771)
[024E0F80:024E4B00 dia] 0x004030DE in void app.runTest() at e:\_docs\_bitbucket\redistest\source\app.d(79)
[024E0F80:024E4B00 dia] 0x00403379 in void app.main().__lambda1() at e:\_docs\_bitbucket\redistest\source\app.d(98)
[024E0F80:024E4B00 dia] 0x0040DB10 in D4vibe4core4core27__T16makeTaskFuncInfoTDFZvZ16makeTaskFuncInfoFNbDFZvZS4vibe4core4core12TaskFuncInfo12callDelegateFPS4vibe4core4core12TaskFuncInfoZv at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\core\core.d(478)
[024E0F80:024E4B00 dia] 0x004228B4 in void vibe.core.core.CoreTask.run() at C:\Users\Stephan\AppData\Roaming\dub\packages\vibe-d-0.7.21-rc.1\source\vibe\core\core.d(959)
[024E0F80:024E4B00 dia] 0x004FEFE9 in void core.thread.Fiber.run()
core.exception.InvalidMemoryOperationError

note that there is actually not running any redis, but nevertheless it should not crash

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 10, 2014

Contributor

@etcimon sorry to bother you with this, but i am kinda lost here. any idea what changed between 0.7.21-alpha.4 and the next tag, that could lead to this ?

Contributor

Extrawurst commented Oct 10, 2014

@etcimon sorry to bother you with this, but i am kinda lost here. any idea what changed between 0.7.21-alpha.4 and the next tag, that could lead to this ?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 10, 2014

Contributor

You weren't running redis?

Contributor

etcimon commented Oct 10, 2014

You weren't running redis?

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 10, 2014

Contributor

@etcimon yes like i said, but this should absolutely not crash. secondly, the code does not crash if redis is running, but the pubsub example does not work at all:

e:\_docs\_bitbucket\redistest>redistest.exe -vvvv
[02520F80:00000000 dia] libevent version: 2.0.20-stable
[02520F80:00000000 dia] libevent is using win32 for events.
PUBLISH Sent: 2014-Oct-10 22:32:05.8506423
LISTEN Stopped: true
Redis Test Succeeded.

no message is received, subscribing does not work.

Contributor

Extrawurst commented Oct 10, 2014

@etcimon yes like i said, but this should absolutely not crash. secondly, the code does not crash if redis is running, but the pubsub example does not work at all:

e:\_docs\_bitbucket\redistest>redistest.exe -vvvv
[02520F80:00000000 dia] libevent version: 2.0.20-stable
[02520F80:00000000 dia] libevent is using win32 for events.
PUBLISH Sent: 2014-Oct-10 22:32:05.8506423
LISTEN Stopped: true
Redis Test Succeeded.

no message is received, subscribing does not work.

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 10, 2014

Contributor

this is the correct output that the exact same example prints with the last working vibe-version alpha4:

e:\_docs\_bitbucket\redistest>redistest.exe -vvvv
[00DD0F80:00000000 dia] libevent version: 2.0.20-stable
[00DD0F80:00000000 dia] libevent is using win32 for events.
Callback subscribe(["SomeChannel"])
PUBLISH Sent: 2014-Oct-10 22:39:14.2142477
LISTEN Recv Channel: SomeChannel, Message: Messageeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
LISTEN Recv Time: 2014-Oct-10 22:39:14.2254032
Callback unsubscribe(["SomeChannel"])
LISTEN Stopped: true
Redis Test Succeeded.
Contributor

Extrawurst commented Oct 10, 2014

this is the correct output that the exact same example prints with the last working vibe-version alpha4:

e:\_docs\_bitbucket\redistest>redistest.exe -vvvv
[00DD0F80:00000000 dia] libevent version: 2.0.20-stable
[00DD0F80:00000000 dia] libevent is using win32 for events.
Callback subscribe(["SomeChannel"])
PUBLISH Sent: 2014-Oct-10 22:39:14.2142477
LISTEN Recv Channel: SomeChannel, Message: Messageeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee
LISTEN Recv Time: 2014-Oct-10 22:39:14.2254032
Callback unsubscribe(["SomeChannel"])
LISTEN Stopped: true
Redis Test Succeeded.
@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 10, 2014

Contributor

Looks like you're unsubscribing immediately after publishing. You should add some sleepers at appropriate places to replicate realistic behavior, I think I had a pull request for that. Because currently, the publish might not have enough time to get through, kind of like a race condition

Contributor

etcimon commented Oct 10, 2014

Looks like you're unsubscribing immediately after publishing. You should add some sleepers at appropriate places to replicate realistic behavior, I think I had a pull request for that. Because currently, the publish might not have enough time to get through, kind of like a race condition

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 10, 2014

Contributor

@etcimon i added those sleeps actually. does the test application actually work for you in the current vibe version with your changes ? like i say, for me it crashes when not running redis server and wont subscribe if redis runs even with pretty long sleeps...

Contributor

Extrawurst commented Oct 10, 2014

@etcimon i added those sleeps actually. does the test application actually work for you in the current vibe version with your changes ? like i say, for me it crashes when not running redis server and wont subscribe if redis runs even with pretty long sleeps...

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 10, 2014

Contributor

I'm going to work on a fix for the access violation issue, I still have a few things to tweak on the other pull request and I was busy with the botan SSL library in writing. how urgent would it be on a scale from 1 to 10?

Contributor

etcimon commented Oct 10, 2014

I'm going to work on a fix for the access violation issue, I still have a few things to tweak on the other pull request and I was busy with the botan SSL library in writing. how urgent would it be on a scale from 1 to 10?

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 10, 2014

Contributor

right now i am pretty stuck with this. pubsub not working at all. its a 8 on that scale and should under no circumstances stay like this in the .21 release

Contributor

Extrawurst commented Oct 10, 2014

right now i am pretty stuck with this. pubsub not working at all. its a 8 on that scale and should under no circumstances stay like this in the .21 release

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 10, 2014

Contributor

It worked for me yes, Travis had confirmed everything. I was focusing mostly on the native event library though. I'll find some time tomorrow to test it out again more thoroughly

Contributor

etcimon commented Oct 10, 2014

It worked for me yes, Travis had confirmed everything. I was focusing mostly on the native event library though. I'll find some time tomorrow to test it out again more thoroughly

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 10, 2014

Contributor

of course travis says it works. like i mentioned right at the beginning, the test code has to be altered to make it misbehave -.-

Contributor

Extrawurst commented Oct 10, 2014

of course travis says it works. like i mentioned right at the beginning, the test code has to be altered to make it misbehave -.-

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 10, 2014

Contributor

Ok, I'm going to repair my pull request, add some more complex pubsub tests, and make sure its ready tomorrow.

Contributor

etcimon commented Oct 10, 2014

Ok, I'm going to repair my pull request, add some more complex pubsub tests, and make sure its ready tomorrow.

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 10, 2014

Contributor

that would be awesome, really. thank you

Contributor

Extrawurst commented Oct 10, 2014

that would be awesome, really. thank you

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 11, 2014

Contributor

Looks like a synchronization error. The connection is created when listening, and when subscribing, both end up colliding together. I intended to protect the m_lockedConnection.conn better, I might use a single mutex instead of m_startMutex and m_capMutex separately. That, and I'll use vibe.core.concurrency message queues rather than TaskConditions

edit: The above was false, the real issue is the listener stays active although the program fails.

Contributor

etcimon commented Oct 11, 2014

Looks like a synchronization error. The connection is created when listening, and when subscribing, both end up colliding together. I intended to protect the m_lockedConnection.conn better, I might use a single mutex instead of m_startMutex and m_capMutex separately. That, and I'll use vibe.core.concurrency message queues rather than TaskConditions

edit: The above was false, the real issue is the listener stays active although the program fails.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 11, 2014

Contributor

I'll have to change RedisSubscriber into a refcounted struct and use bstop() in the destructor.

Contributor

etcimon commented Oct 11, 2014

I'll have to change RedisSubscriber into a refcounted struct and use bstop() in the destructor.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 11, 2014

Contributor

I have a couple options, I'm not sure which is best:

  • I can add in the RedisSubscriber constructor or invariant an assert(!GC.getAttr(this), "The object cannot be GC allocated because ~this() must be run for the listener tasks to stop when an error is thrown.").
  • I can rename it to RedisSubscriberImpl and make it reference counted, to guarantee that ~this is triggered when it goes out of scope
Contributor

etcimon commented Oct 11, 2014

I have a couple options, I'm not sure which is best:

  • I can add in the RedisSubscriber constructor or invariant an assert(!GC.getAttr(this), "The object cannot be GC allocated because ~this() must be run for the listener tasks to stop when an error is thrown.").
  • I can rename it to RedisSubscriberImpl and make it reference counted, to guarantee that ~this is triggered when it goes out of scope
@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 11, 2014

Contributor

The second option sounds like a more clean approach, right ? on the other hand i see myself more like a user of the library, i am not too much familiar with its inner designs, i just desperately need it to work like it was a couple of tags ago...

Contributor

Extrawurst commented Oct 11, 2014

The second option sounds like a more clean approach, right ? on the other hand i see myself more like a user of the library, i am not too much familiar with its inner designs, i just desperately need it to work like it was a couple of tags ago...

etcimon added a commit to etcimon/vibe.d that referenced this issue Oct 11, 2014

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 11, 2014

Contributor

This should be as stable as it gets, I attempted thread-safety as well. Didn't have time to make the tests more complex, I'll get around to it ;) Hope it works out for you!

Contributor

etcimon commented Oct 11, 2014

This should be as stable as it gets, I attempted thread-safety as well. Didn't have time to make the tests more complex, I'll get around to it ;) Hope it works out for you!

etcimon added a commit to etcimon/vibe.d that referenced this issue Oct 12, 2014

etcimon added a commit to etcimon/vibe.d that referenced this issue Oct 12, 2014

etcimon added a commit to etcimon/vibe.d that referenced this issue Oct 12, 2014

etcimon added a commit to etcimon/vibe.d that referenced this issue Oct 12, 2014

etcimon added a commit to etcimon/vibe.d that referenced this issue Oct 12, 2014

etcimon added a commit to etcimon/vibe.d that referenced this issue Oct 12, 2014

etcimon added a commit to etcimon/vibe.d that referenced this issue Oct 12, 2014

Fix Redis issue #855
Added support for events outside tasks

Added error handling

Revert "Updated json to correctly handle double and float nan"

This reverts commit 046eac1.

Trailing whitespace

etcimon added a commit to etcimon/vibe.d that referenced this issue Oct 12, 2014

etcimon added a commit to etcimon/vibe.d that referenced this issue Oct 12, 2014

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 12, 2014

Contributor

my tests look good so far. thank you so much

Contributor

Extrawurst commented Oct 12, 2014

my tests look good so far. thank you so much

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 12, 2014

Contributor

No problem! Code reviews are the main reasons why I write open source in the first place ;)

Contributor

etcimon commented Oct 12, 2014

No problem! Code reviews are the main reasons why I write open source in the first place ;)

@s-ludwig s-ludwig added this to the 0.7.21 milestone Oct 13, 2014

@s-ludwig s-ludwig closed this Oct 13, 2014

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Oct 13, 2014

Contributor

@s-ludwig thank you for merging, can you please tag again ?

Contributor

Extrawurst commented Oct 13, 2014

@s-ludwig thank you for merging, can you please tag again ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment