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

Fix compilation on DMD v2.067 #992

Merged
merged 1 commit into from Feb 27, 2015

Conversation

Projects
None yet
5 participants
@etcimon
Contributor

etcimon commented Feb 25, 2015

This also adds a fix for HashMap reallocation issue pointed out earlier.

Show outdated Hide outdated source/vibe/core/sync.d
import libasync.threads : destroyAsyncThreads;
destroyAsyncThreads(); // destroy threads
}
}

This comment has been minimized.

@Geod24

Geod24 Feb 25, 2015

Contributor

IIUC, this (and code at line 1349) is similar to what was in libasync (at line 1575). Why is the change needed ?
IMO driver-specific-code should go into core, unless there's no other way.

@Geod24

Geod24 Feb 25, 2015

Contributor

IIUC, this (and code at line 1349) is similar to what was in libasync (at line 1575). Why is the change needed ?
IMO driver-specific-code should go into core, unless there's no other way.

This comment has been minimized.

@etcimon

etcimon Feb 25, 2015

Contributor

IIUC, this (and code at line 1349) is similar to what was in libasync (at line 1575). Why is the change needed ?
IMO driver-specific-code should go into core, unless there's no other way.

I get a runtime circular reference (circular imports?) error in druntime if I leave it in the driver. This is really the only way I've found, and it's a necessity now.

@etcimon

etcimon Feb 25, 2015

Contributor

IIUC, this (and code at line 1349) is similar to what was in libasync (at line 1575). Why is the change needed ?
IMO driver-specific-code should go into core, unless there's no other way.

I get a runtime circular reference (circular imports?) error in druntime if I leave it in the driver. This is really the only way I've found, and it's a necessity now.

This comment has been minimized.

@Geod24

Geod24 Feb 25, 2015

Contributor

Was it present in 2.066 (and before) or does it comes with 2.067 ?

@Geod24

Geod24 Feb 25, 2015

Contributor

Was it present in 2.066 (and before) or does it comes with 2.067 ?

This comment has been minimized.

@etcimon

etcimon Feb 25, 2015

Contributor

It follows a fix I made in libasync, where it checks that daemon threads are destroyed when exiting to avoid some invalid memory operations. So everything using a newer version of libasync gets an assertion error on exit unless the threads are destroyed properly. I couldn't destroy them in the driver in any DMD version anyways.

@etcimon

etcimon Feb 25, 2015

Contributor

It follows a fix I made in libasync, where it checks that daemon threads are destroyed when exiting to avoid some invalid memory operations. So everything using a newer version of libasync gets an assertion error on exit unless the threads are destroyed properly. I couldn't destroy them in the driver in any DMD version anyways.

@@ -601,7 +601,7 @@ struct CookieValueMap {
inout(string)* opBinaryRight(string op)(string name) inout if(op == "in")
{
foreach(c; m_entries)
foreach(ref c; m_entries)

This comment has been minimized.

@Geod24

Geod24 Feb 25, 2015

Contributor

This shouldn't be needed. I did the same change in #972, but Sonke fixed it in 29d3fad

@Geod24

Geod24 Feb 25, 2015

Contributor

This shouldn't be needed. I did the same change in #972, but Sonke fixed it in 29d3fad

This comment has been minimized.

@etcimon

etcimon Feb 25, 2015

Contributor

This gives me an error about escaping local references in the newer DMD. I was working with git head so any fix that was there wasn't working

@etcimon

etcimon Feb 25, 2015

Contributor

This gives me an error about escaping local references in the newer DMD. I was working with git head so any fix that was there wasn't working

This comment has been minimized.

@s-ludwig

s-ludwig Feb 26, 2015

Member

Does your current code here compile with DMD 2.065? It gave an error about not being able to convert from inout to inout or similar, which is why the static if my fixing commit was still necessary.

@s-ludwig

s-ludwig Feb 26, 2015

Member

Does your current code here compile with DMD 2.065? It gave an error about not being able to convert from inout to inout or similar, which is why the static if my fixing commit was still necessary.

This comment has been minimized.

@etcimon

etcimon Feb 26, 2015

Contributor

Travis seems to suggest everything is good all the way down to libevent 2.065 so this seems like the proper way to go

@etcimon

etcimon Feb 26, 2015

Contributor

Travis seems to suggest everything is good all the way down to libevent 2.065 so this seems like the proper way to go

@@ -102,7 +102,7 @@ template findNextUDA(UDA, alias Symbol, long idx, bool allow_types = false)
static assert(idx >= 0, "Index given to findNextUDA can't be negative");
static assert(idx <= udaTuple.length, "Index given to findNextUDA is above the number of attribute");
private template extract(size_t index, list...)
public template extract(size_t index, list...)

This comment has been minimized.

@Geod24

Geod24 Feb 25, 2015

Contributor

Looks like a regression to me. I encountered it in #972 but didn't have time to look into this. I'll see if I can repro / submit an issue tomorrow afternoon.

@Geod24

Geod24 Feb 25, 2015

Contributor

Looks like a regression to me. I encountered it in #972 but didn't have time to look into this. I'll see if I can repro / submit an issue tomorrow afternoon.

This comment has been minimized.

@etcimon

etcimon Feb 25, 2015

Contributor

It allows everything to compile on my end, so, I'm happy with it :-)

@etcimon

etcimon Feb 25, 2015

Contributor

It allows everything to compile on my end, so, I'm happy with it :-)

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 26, 2015

Member

Still fails, because libasync.all doesn't exist.

Member

s-ludwig commented Feb 26, 2015

Still fails, because libasync.all doesn't exist.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 26, 2015

Contributor

Still fails, because libasync.all doesn't exist.

Yes, that one was removed recently in favor of just libasync

Contributor

etcimon commented Feb 26, 2015

Still fails, because libasync.all doesn't exist.

Yes, that one was removed recently in favor of just libasync

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Feb 26, 2015

Contributor

It's in need for a rebase.

I still don't consider 7cca2ba as a good solution though, especially given the fact it turns an Exception into an Error.

Contributor

Geod24 commented Feb 26, 2015

It's in need for a rebase.

I still don't consider 7cca2ba as a good solution though, especially given the fact it turns an Exception into an Error.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 26, 2015

Contributor

I think the proper solution is to stop inheriting from Object.Monitor, stop using synchronized in fibers, and write a scopedLock!TaskMutex for it instead (so that fibers can be interrupted!)

Contributor

etcimon commented Feb 26, 2015

I think the proper solution is to stop inheriting from Object.Monitor, stop using synchronized in fibers, and write a scopedLock!TaskMutex for it instead (so that fibers can be interrupted!)

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Feb 26, 2015

Contributor

turns an Exception into an Error

Into segmentation fault actually (with -release) ;)

Contributor

mihails-strasuns commented Feb 26, 2015

turns an Exception into an Error

Into segmentation fault actually (with -release) ;)

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 26, 2015

Contributor

Into segmentation fault actually (with -release) ;)

I got a strange segmentation fault in -release using dmd master with libasync (Windows). This isn't supposed to happen, what platform are you on.

Contributor

etcimon commented Feb 26, 2015

Into segmentation fault actually (with -release) ;)

I got a strange segmentation fault in -release using dmd master with libasync (Windows). This isn't supposed to happen, what platform are you on.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 26, 2015

Contributor

Nevermind, that's actually what usually happens on assertion failures in release ^^

Contributor

etcimon commented Feb 26, 2015

Nevermind, that's actually what usually happens on assertion failures in release ^^

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 26, 2015

Contributor

I hope this commit will settle the question :)

Contributor

etcimon commented Feb 26, 2015

I hope this commit will settle the question :)

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 26, 2015

Member

Sorry, but in this form this is not a solution. This is all public code and changing the inheritance like this is a breaking change. All breaking changes must at least go through a deprecation phase.

Member

s-ludwig commented Feb 26, 2015

Sorry, but in this form this is not a solution. This is all public code and changing the inheritance like this is a breaking change. All breaking changes must at least go through a deprecation phase.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 26, 2015

Contributor

All breaking changes must at least go through a deprecation phase.

How do you deprecate broken code (on DMD 2.067)? Unless I wrap it in a static if version clause?

Contributor

etcimon commented Feb 26, 2015

All breaking changes must at least go through a deprecation phase.

How do you deprecate broken code (on DMD 2.067)? Unless I wrap it in a static if version clause?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 26, 2015

Contributor

This new fix adds some features when running DMD <= 2.066, and removes other features for DMD 2.067 to make vibe.d compatible with it.

Unfortunately, it'll break anything that tries to run on DMD 2.067 with certain mutex uses, but that was already broken and is simply bubbling up from druntime.

I don't know how to deprecate it properly, but the changes for DMD >= 2.067 are as follows:

  • TaskMutex, RecursiveTaskMutex, TaskCondition are now detached from Object.Monitor on the inheritance tree
  • synchronized statement is unavailable as a consequence, it must be replaced with auto l = mutex.locked, or auto l = scopedLock(mutex) which returns a ScopedMutexLock
  • If using DMD >= 2.067, the with statement is also available as a convenience (it doesn't work on <= 2.066): with(mutex.locked) { /* synchronized */ }
Contributor

etcimon commented Feb 26, 2015

This new fix adds some features when running DMD <= 2.066, and removes other features for DMD 2.067 to make vibe.d compatible with it.

Unfortunately, it'll break anything that tries to run on DMD 2.067 with certain mutex uses, but that was already broken and is simply bubbling up from druntime.

I don't know how to deprecate it properly, but the changes for DMD >= 2.067 are as follows:

  • TaskMutex, RecursiveTaskMutex, TaskCondition are now detached from Object.Monitor on the inheritance tree
  • synchronized statement is unavailable as a consequence, it must be replaced with auto l = mutex.locked, or auto l = scopedLock(mutex) which returns a ScopedMutexLock
  • If using DMD >= 2.067, the with statement is also available as a convenience (it doesn't work on <= 2.066): with(mutex.locked) { /* synchronized */ }
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 26, 2015

Member

What would have to be deprecated is the old API. This would for example be the whole TaskMutex class. A new class would then have to be added as a replacement. But maybe there is a more elegant path, too... maybe using some alias this trickery. But the final direction taken in Druntime needs to be clear first.

BTW, TaskMutex.locked is redundant and doesn't save much over m.scopedLock (UFCS). I'd remove it to avoid confusion.

Member

s-ludwig commented Feb 26, 2015

What would have to be deprecated is the old API. This would for example be the whole TaskMutex class. A new class would then have to be added as a replacement. But maybe there is a more elegant path, too... maybe using some alias this trickery. But the final direction taken in Druntime needs to be clear first.

BTW, TaskMutex.locked is redundant and doesn't save much over m.scopedLock (UFCS). I'd remove it to avoid confusion.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 26, 2015

Contributor

Didn't think the UFCS would allow me to remove parenthesis.

So this last fix should be good, if you can merge the travis exclusion I'm sure we can see the whole thing go green.

Contributor

etcimon commented Feb 26, 2015

Didn't think the UFCS would allow me to remove parenthesis.

So this last fix should be good, if you can merge the travis exclusion I'm sure we can see the whole thing go green.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 26, 2015

Member

I would have liked to keep the mutex changes separate from the libasync ones (defer it until the Druntime case is settled), but anyway, that can always be changed again before release. But the first commit still seems to contain merge errors (reverts some changes in the libevent driver), or was that intended (Travis still passes)?

Member

s-ludwig commented Feb 26, 2015

I would have liked to keep the mutex changes separate from the libasync ones (defer it until the Druntime case is settled), but anyway, that can always be changed again before release. But the first commit still seems to contain merge errors (reverts some changes in the libevent driver), or was that intended (Travis still passes)?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 26, 2015

Contributor

But the first commit still seems to contain merge errors (reverts some changes in the libevent driver)

That was intentional, the changes I submitted are in a different direction and the nothrow / static assert(false) are unnecessary now. This fix doesn't affect the DMD <= 2.066.1 builds

I would have liked to keep the mutex changes separate from the libasync ones (defer it until the Druntime case is settled)

They allow libasync to run on 2.067, which is relevant to the pull :)

Contributor

etcimon commented Feb 26, 2015

But the first commit still seems to contain merge errors (reverts some changes in the libevent driver)

That was intentional, the changes I submitted are in a different direction and the nothrow / static assert(false) are unnecessary now. This fix doesn't affect the DMD <= 2.066.1 builds

I would have liked to keep the mutex changes separate from the libasync ones (defer it until the Druntime case is settled)

They allow libasync to run on 2.067, which is relevant to the pull :)

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 26, 2015

Contributor

(Travis still passes)?

Travis passes on everything except the 4 versions that aren't supported by libasync (yet)

Contributor

etcimon commented Feb 26, 2015

(Travis still passes)?

Travis passes on everything except the 4 versions that aren't supported by libasync (yet)

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 26, 2015

Member

So did you make the changes in libevent.d on purpose? Why are they in the commit that fixes the libasync driver?

Member

s-ludwig commented Feb 26, 2015

So did you make the changes in libevent.d on purpose? Why are they in the commit that fixes the libasync driver?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 26, 2015

Contributor

So did you make the changes in libevent.d on purpose? Why are they in the commit that fixes the libasync driver?

They're in the libasync driver pull because it must address the whole of the mutex issues in 2.067, where the proposed solution was to make the drivers nothrow. The libevent driver had a few added nothrows, which I removed because it seemed unnecessary given the fact that the new approach is to let the mutexes and drivers remain throwable.

I can revert the changes if you like.

Contributor

etcimon commented Feb 26, 2015

So did you make the changes in libevent.d on purpose? Why are they in the commit that fixes the libasync driver?

They're in the libasync driver pull because it must address the whole of the mutex issues in 2.067, where the proposed solution was to make the drivers nothrow. The libevent driver had a few added nothrows, which I removed because it seemed unnecessary given the fact that the new approach is to let the mutexes and drivers remain throwable.

I can revert the changes if you like.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 26, 2015

Member

I can revert the changes if you like.

No no, I think it's most probably a step in the right direction. It just seemed out of place in the commit order back then.

Member

s-ludwig commented Feb 26, 2015

I can revert the changes if you like.

No no, I think it's most probably a step in the right direction. It just seemed out of place in the commit order back then.

Fixed libasync on new dmd
Made libevent default again

Fix dub.json

Remove artifact

Add assertions to replace caught exceptions

Update version for travis tests

Remove libasync.all

Add sorting import for redis test

Implement throwable task mutexes

Removed memory changes

fix segfault

Lock using with statement in synchronized-like syntax

Add compatibility with previous DMD versions

Add deprecation messages for TaskMutex & RecursiveTaskMutex

update test
@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Feb 27, 2015

Contributor

Green!

Contributor

etcimon commented Feb 27, 2015

Green!

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 27, 2015

Member

Yay!

Member

s-ludwig commented Feb 27, 2015

Yay!

s-ludwig added a commit that referenced this pull request Feb 27, 2015

@s-ludwig s-ludwig merged commit c3e0a8c into vibe-d:master Feb 27, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 27, 2015

Member

And thanks of course!

Member

s-ludwig commented Feb 27, 2015

And thanks of course!

@etcimon etcimon deleted the etcimon:fix-libasync-new-dmd branch Feb 27, 2015

@trusted bool tryLock();
@trusted void lock();
@trusted void unlock();
}

This comment has been minimized.

@MartinNowak

MartinNowak Mar 4, 2015

Contributor

This won't work with synchronized and really looks like an abysmal solution.

@MartinNowak

MartinNowak Mar 4, 2015

Contributor

This won't work with synchronized and really looks like an abysmal solution.

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